-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add patches for LLVM 3.8.0 #15417
Add patches for LLVM 3.8.0 #15417
Conversation
f7c024b
to
ee9f74f
Compare
And make ORC JIT the default for those LLVM versions. This does not activate the new LLVM versions by default yet, to give time to update everything on CI, etc., but doing the actual activation is a one line change after this.
Also http://reviews.llvm.org/D17165 or I believe the backtrace tests in replutil will fail. |
Haven't tried this anywhere other than win32 yet. If you want to push that patch here before I can get to it, be my guest. |
Indeed, I confirm the replutil test fails with vanilla LLVM 3.8. Currently retrying with the patch. |
ee9f74f
to
2938120
Compare
Confirmed on Linux, patch added. Somebody should email LLVM's release manager and ask for D17165 (rL260791) to be backported to the release_38 branch so it makes it into 3.8.1. |
RPM builds went fine with the patch (except for one random failure). |
Win64 actually segfaults in the subarray test, so something is wrong here. Happens with or without the D17165 patch though. |
Is it repeatable? Reproducible with single test? Possible to get a backtrace? =) |
Yes, yes, taking forever under gdb so don't know yet. |
Is it expected that something that takes 10 minutes outside gdb would take 12 hours and counting inside it? #14846 maybe? |
yes, that's a known bug in gdb |
Does LLDB work on Windows or should I delta-debug |
Maybe you can "bisect" the test to see if there's a smaller repro. |
That's what delta debugging is. The technique predates bisecting and is where git et al got the idea from. |
If you don't need to debug jitted code, you can patch around the gdb performance problem with --- a/src/jitlayers.cpp
+++ b/src/jitlayers.cpp
@@ -187,7 +187,7 @@ void NotifyDebugger(jit_code_entry *JITCodeEntry)
}
__jit_debug_descriptor.first_entry = JITCodeEntry;
__jit_debug_descriptor.relevant_entry = JITCodeEntry;
- __jit_debug_register_code();
+ //__jit_debug_register_code();
} |
2938120
to
c1e81c6
Compare
Turned out not too bad to reduce it:
|
Does not segfault at |
Anything holding back this PR? There has been frequent breakage on llvm 3.8 recently and while we may not be able to use llvm 3.8 by default due to windows (or whatever) issues, having this merged will make testing easier. |
Doesn't hurt to merge it I suppose. I'll test again to see whether the segfault is still an issue. edit: doesn't build right now
|
c1e81c6
to
12c18c9
Compare
12c18c9 fixes the build failure, but |
The debug info patch should be replaced/combined with the one in http://reviews.llvm.org/D18583 . |
376436b
to
72d2739
Compare
Okay, done but do double-check. Needs a dependency since the patches touch the same files. Can you contact the LLVM release manager and ask for these things to be backported to the release_38 branch so they are included in 3.8.1?
|
Might actually be easier to merge the two. Otherwise LGTM.
Not sure about the right procedure and I thought @Keno have already done that? |
I never made a formal request, so it probably got list. Do note that |
Nevermind, the patch that replaces it is D18583, which is the second patch here. |
@@ -414,7 +414,7 @@ LLVM_FLAGS += --enable-libcpp | |||
endif # USE_LIBCPP | |||
ifeq ($(OS), WINNT) | |||
LLVM_FLAGS += --with-extra-ld-options="-Wl,--stack,8388608" LDFLAGS="" | |||
LLVM_CPPFLAGS += -D__USING_SJLJ_EXCEPTIONS__ -D__CRT__NO_INLINE | |||
LLVM_CPPFLAGS += -D__USING_SJLJ_EXCEPTIONS__ -D__CRT__NO_INLINE -DMINGW_HAS_SECURE_API=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this for? according to google, this flag will cause the addition of a dependency on msvcrt80+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
citation needed. the 32 bit mingw-w64 compilers used to be built without the secure api enabled by default in the cygwin packaging. that's been fixed now, but requires updating the cygwin packages on the buildbots before we can remove this. @staticfloat what's the process for updating cygwin on the windows buildbots?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the so-called secure api is a Windows Vista / msvcrt80+ feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's implemented in mingw-w64's runtime. Only works on Vista or newer, but LLVM's unit tests are now assuming it's present without checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't you logged in through cygwin's ssh though? to really update everything, you need to update cygwin without any cygwin applications running
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly enough, cygwin will kill your ssh session, and continue the update. I then restart the server through Horizon, and it seems to work just fine. A cleaner solution is to login over RDP and do everything in cmd.exe of course, but that's a much bigger hassle because it requires you to have the auto-generated windows password written down, etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clever cygwin. works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did that manage to break codesigning? the win32 buildbot apparently needs to be woken back up. what is Horizon and could you send me credentials via email so I can do some of these remote admin things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming at you over email.
remove secure api flag that isn't needed if cygwin is up to date
Add http://reviews.llvm.org/D18583 as well Concatenate the two patches together
8b73786
to
5746e1f
Compare
I concatenated the two patches together, hopefully that'll work. Still help wanted on the win64 subarray segfault, but if this is green then it shouldn't hurt anything to merge it. |
For the segfault, the backtrace suggests that it happens in |
There's also a new segfault in bitarray:
|
checking we emitted:
llvm emitted:
(where %r15 was %3 is a 16-byte aligned jl_value_t*) |
may 25th is going to be the deadline for requesting backports into llvm 3.8.1: http://lists.llvm.org/pipermail/llvm-dev/2016-April/098637.html |
The bitarray segfault went away with #16011, maybe that just changes the code paths getting hit though? |
I'm seeing a similar segfault on linux x64 too. This seems to be a LLVM 3.8 bug when splitting an aligned aggregate load into scalar loads. It transforms %26 = load %StepRange.12, %StepRange.12* %25, align 16, !tbaa !6 into %.elt = getelementptr inbounds %StepRange.12, %StepRange.12* %16, i64 0, i32 0
%.unpack = load i64, i64* %.elt, align 16
%.elt2 = getelementptr inbounds %StepRange.12, %StepRange.12* %16, i64 0, i32 1
%.unpack3 = load i64, i64* %.elt2, align 16
%.elt4 = getelementptr inbounds %StepRange.12, %StepRange.12* %16, i64 0, i32 2
%.unpack5 = load i64, i64* %.elt4, align 16 which is impossible since the three addresses separated by 8 cannot be all 16 bytes aligned.... This seems to be fixed on 3.9 so there's hope that there's a patch that we can backport... |
I think everything in
llvm-3.7.1.patch
andllvm-3.7.1_2.patch
made it upstream for 3.8.0, aside from the DLL Makefile fix. Will have to test with cmake + shared libraries on Windows too.I'm borrowing a patch from Rust to fix compilation with the win32-threads mingw-w64 variant that we've been using, ref rust-lang/rust#30448 (comment) and rust-lang/llvm@69ef168. If we do want to use of the parallel codegen support in LLVM (when possible) that this patch is disabling, we can make the patch Windows-only.