- User Since
- Jun 28 2018, 11:39 AM (46 w, 3 d)
Tue, May 14
So, while I think this is an entirely reasonable assumption in most cases, it's also not one that provides any kind of workaround for the few cases where it's not universally true.
Fri, May 10
Mon, May 6
I had to revert this in rL360086. I'm following up offline to see if I can narrow down what the issue is.
Fri, May 3
I didn't follow the technical details, but I don't see anything wrong with moving forward on this patch. I think this seems like an interesting idea worth experimenting with.
Thu, May 2
Looks like support was recently added in rL359380, though without any tests, and was accidentally reverted.
Reverted in r359831 due to a dependency in reverting r359311
Reverted in r359830 due to a dependency in reverting r359311
Reverted in r359830, this regresses GNU compatibility.
(Also sorry for the delay; I was out last week and am doing a very slow job of un-burying myself from what's piled up)
Adding zbrid since she has context already. I'll take a look in a bit too. Thanks for the patch!
Tue, Apr 30
llvm-objcopy test looks fine
I think this is a reasonable change -- it isn't strictly matching GNU objcopy in my tests, but we can update it if we find out this edge case is important.
Mon, Apr 29
Apr 18 2019
Just to see if I understand the comments so far: --special-syms is essentially already the default for llvm-nm (i.e. we print special symbols), but not for GNU nm. And so even though we print special symbols, a configure script that checks if $NM --special-syms is supported will fail when it shouldn't.
Apr 17 2019
Sorry, this patch dropped off my radar. I'm not sure I'll get to it this or next week, but I'll make sure to review it eventually (unless ruiu does it sooner).
Sorry, I should have just accepted this before -- the option missing from strip is my only objection. The rest are just suggestions.
lgtm, just a couple minor comments
- Use %b format to avoid whitespace handling
- Handle ref names
Apr 16 2019
Ok, this helps me see what's going on. Thanks!
- Fix typos
- Use alternative constructor to avoid specifying OSABI_NONE all the time
Apr 15 2019
Thanks for adding this!
Part of me thinks this is bad -- if the version of glibc is old, then the host toolchain should just upgrade it. glibc 2.23 was released in 2016-02-19, so I think using something older than that is on the border of being called "not a modern c++ toolchain".
(requesting changes for one small bit, otherwise lgtm)
Apr 9 2019
I'm fine with the -w flag, but I haven't had time to look at tab alignment code -- it seems very hard to follow. Perhaps it should be a separate patch? This seems like two unrelated changes.
I'm out the rest of the week, I'll leave it to James to do the commit on your behalf
Apr 4 2019
Thanks for sending this! I assigned PR39841 to you since this should fix that bug.
Just a couple small comments, but also LGTM
Apr 3 2019
Also don't we already have an update sections loop somewhere in this already? I think this should go there to reduce the number of passes we make over the sections
I feel somewhat ambivalent about this change. The rationale makes sense (great patch description!), but these section flags provide GNU objcopy compatibility, so I'm not sure making it do things the "expected" way is actually helpful -- we should just provide separate flags if we want to do things the right way. (e.g. expose "sht_progbits" as a command line flag).
Thanks for splitting this change off.
Apr 2 2019
From the bug in the patch description, this is used by linux here: https://github.com/ClangBuiltLinux/linux/blob/master/drivers/firmware/efi/libstub/Makefile#L75
LGTM, but since this is a pretty big change, it'd be good to get someone else's opinion
Apr 1 2019
Looks good to me, thanks! Do you need one of us to commit on your behalf?
- Fix some method/variable names
(adding a few others that may be interested)
Code looks good, but can you add a test for this? Adding something to test/tools/llvm-objcopy/ELF/keep-symbol.test should be fine
Mar 29 2019
- Only change the type if the input is SHT_NOBITS
- Add test coverage
- Update test comments