- User Since
- Jun 28 2018, 11:39 AM (133 w, 3 d)
Wed, Jan 13
This patch seems to have found a bug: in AArch64InstrInfo.cpp, there is a -Wsometimes-uninitialized warning that now triggers, and adding an assertion to make sure the value is eventually set causes a few tests to fail. The usage is DelInstrs.push_back(MUL), which I guess is how it ties in to this patch.
Mon, Jan 11
I ran into this commit when integrating commits today (specifically, 97100646d1b4526de1eac3aacdb0b098739c6ec9) -- there's nothing wrong with this patch AFAICT, but I'm wondering if the error messaging/handling could be improved somehow.
Wed, Jan 6
From a user standpoint, I agree that "objcopy foo.o" should not be changing the ownership of a file, regardless of the implementation (which, in the case of llvm-objcopy, is getting it "wrong" compared to GNU objcopy, because we use temporary file and rename it at the end, without fixing ownership). Sounds like there's agreement there.
I can confirm this fixes the non-reduced test case we're seeing internally, thanks!
Wed, Dec 30
Finally got a reasonably sized reduction. It's probably so long because I'm just using clang to build, and if it gets smaller than clang optimizes too much of it away; if I used opt to control the passes I could probably get it smaller. Anyway, hope this is good enough to demonstrate the issue:
Mon, Dec 28
I think I'm seeing a miscompile as a result of the patch. The issue is not the transformation itself, but a dependent instruction is reading the wrong value.
Wed, Dec 23
Dec 17 2020
This now "fixes" the dwarf 5 crash I originally went down this rabbit hole for, but I'd like feedback on if it's actually the correct fix (e.g. I'm not generating bogus debug info here :) )
- Fix the dwarf 5 crash
- Fix the other crash
Do you mean clang -fsplit-dwarf-inlining -fdebug-info-for-profiling should fail during command line parsing? Or something else?
- Update test case to check dwarfdump structure
Dec 16 2020
Dec 4 2020
Nov 25 2020
FYI, this patch (unintentionally?) fixed a miscompile for us, as I noted in the review thread for D90648. Reposting here for visibility:
Turns out this isn't failing at trunk anymore. It looks like this was fixed by 02fdbc3567249471349474c70828cb5a5d4881c8. For posterity, here's my reduction:
Nov 24 2020
LGTM, though it'd be good to have another set of eyes on this to approve it.
Nov 23 2020
Nov 20 2020
I wrote D49514 a long time ago, and since then, llvm-libc was created -- and it looks like it implements fmax, though not scalbn. Anyway, I wonder if it would be possible to use the llvm-libc implementation of fmax instead of adding one here? And similarly, for scalbn, maybe it would be easier/better long term to add one to libc instead of here.
Nov 19 2020
I ran manual tests for this, but I did so by introducing an intentional crash in a place that obviously can't be checked in. Does LLDB have any kind of intentional-crash-for-test mechanism that could be used for a test?
Nov 16 2020
Nov 13 2020
My cmake config appears to be correct. I tried adding this to CommandInterpreter::HandleCommand:
Thanks, I'll take a look at that.
I'm not really sure what you mean by the LLDB backtrace or how this is inhibiting it.
Nov 3 2020
Nov 2 2020
Oct 30 2020
No worries, it was only a minute to patch, and then a few more minutes to take a snack break while it rebuilt :)
Oct 29 2020
I can confirm this fixes the breakage -- thanks!
LGTM. I'll wait for a second person to stamp it.
Sure, running tests now. You can get commit access via http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access for future patches :)
Looks like it was D89384 that got rid of this, fwiw
I'm seeing failures which I think are due to this patch -- I don't have a nice godbolt repro yet, but it's something like:
Oct 19 2020
It looks like this fix caused a different regression in not accepting name.<x>.h as the main header for name.<x>.cc, e.g.:
Oct 12 2020
Sorry for the delay -- yep, going this route (for now, at least) is fine with me too.
Oct 9 2020
Oct 7 2020
Same here -- that seems like it would be a better patch.
Oct 6 2020
Oct 5 2020
Oct 2 2020
Can you also update install-name-tool-delete-rpath.test with a test case for this?
Oct 1 2020
LGTM, thanks again!
D88288 landed this morning. Does that work for you?
Sep 30 2020
I'm really happy to see this. I tried removing the error() methods from llvm-objcopy.h a year or two ago and didn't have the patience to work through all the plumbing seen here.
Sep 29 2020
Sep 28 2020
Requesting changes mostly because this is marked as accepted but I still see build errors; overall this looks fine though.
Sep 25 2020
There's a build warning that needs to be fixed, but otherwise LG.
Sep 23 2020
- Switch back to using localhost for non-socket uses
Sep 22 2020
The issue is with the new pass manager.
Sep 21 2020
Sep 17 2020
Looks like llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test covers this, so LGTM
Sep 16 2020
Sep 15 2020
The initial dns lookup may still fail, I think? e.g. my initial version of this patch had:
- Compare directly against the error code instead of hopping through std::make_error_code()
Sep 14 2020
Sep 10 2020
Sep 9 2020
This is ready for review now after scrubbing out the "localhost" changes I had earlier.
- Don't use "localhost" to avoid dns latency. Instead, prefer either 127.0.0.1 or ::1 directly.