- User Since
- Jan 18 2017, 2:49 AM (98 w, 4 d)
Fri, Dec 7
LGTM for this change. The refactor is certainly good. I feel like I might have suggested this in a previous review a while back, but it obviously never happened (see D49016).
Thu, Dec 6
The output from llvm-readobj vs. llvm-readelf is almost always different and that is either by design or accident. Are you suggesting syncing the output between readobj and readelf for this case, basically setting, --elf-output-style=GNU when --headers is specified. Is that what you want?
We shouldn't sync the output between llvm-readobj and llvm-readelf, although both should produce the right set of headers when requested. For llvm-readelf, we should match what GNU readelf does. llvm-readobj does not need to match this.
Wed, Dec 5
Please remember to include the context when creating the diff with Phabricator (see the section starting "To get a full diff" here.
LGTM. Thinking about it a bit more, and I don't think using the cache will cause any actual problems if everything has the same access time, beyond potentially doing unnecessary work.
Tue, Dec 4
What would happen if the thin LTO cache is used on such a system? Do we need to put something in the thin LTO code to emit some sort of error, if this is attempted?
LGTM, with one minor comment.
Looks good, with a couple of nits.
Mon, Dec 3
LGTM, with a couple of minor comments.
I'm not really a COFF expert, so I haven't yet looked at the details of this change. It broadly looks like the right direction though.
I'm still not particularly familiar with yaml2obj, so my comments might not always make sense, so feel free to correct me.
Thu, Nov 29
Sorry, I've been a bit busy over the last couple of days. Where can I find documentation on these two sections you are adding support for?
Tue, Nov 27
I think it would make sense to test in both cases, honestly, but I'm not too fussed.
Please do split this into a separate pass-through change and option-hook up. It will keep the scale of the review down by quite a bit. Hooking up the options one or two at a time after that would then make reviewing each of those easier too.
Should we do the same thing (adding "comptabile with GNU objcopy") for llvm-objcopy as well as llvm-strip?
Mon, Nov 26
Whilst I'm not opposed to this in principle, I do wonder if the more correct thing to do is to make .gnu.version_r sections (which I assume have a distinctive ELF type) another kind of ELFYAML section (e.g. like RelocationSection).
Tue, Nov 20
Could you add tests for the cases where a) there is no DT_VERNEEDNUM tag, and b) where it has a value of 0, please.
I'd be interested to hear yours and other people's thoughts on not calling reportError inside the MachO layer and propagating it out at least to executeObjcopyOnBinary, via llvm::Error/Expected. I know @jakehehrlich was talking about this in the ELF side at one point too.
Mon, Nov 19
Functionality wise, this seems like a reasonable idea to me. Personally, I think I'd prefer --noinhibit-exec to more closely match the BFD approach of still emitting an actual error, but that's orthogonal to the issue this is trying to solve.
I don't know anything about the MachO file format really, so I'm not sure I'm well qualified to review the file format side of this. I've made a couple of drive-by nitpick comments though, and may come back later with more substantial comments.
Thu, Nov 15
Sorry for not getting to this earlier. It's been a busy week for me.
Wed, Nov 14
I don't really mind either way. Don't make wholesale changes to coding styles to update it to LLVM's style though. If you want to change anything, I'd just change printELFFileHeader, but I don't think it's really worth it unless you are changing the area of code already.
If anything, the header should be updated to use "Obj" for one or two of the other cases. "o" does not fit LLVM's current coding standards, and also does not match the name used in the .cpp file (see ELFDump.cpp).
Just as a tip, don't forget to put the Differential Revision field in your commit message. Then, when you commit it, the commit will automatically get associated with Phabricator, and you don't need to do anything manually, like close the revision or update the final diff. There are some instructions here: https://llvm.org/docs/Phabricator.html#committing-a-change. Also, take a look at commits made by various other people as well based on Phabricator reviews.
LGTM. Do you need it committing? You've probably had enough commits by now that you could probably request commit access, if you want.
Nov 9 2018
I specifically asked for this test to be written this way.
Okay, from my point of view, LGTM (with a nit). However, we should probably get some more buy-in on the RFC before breaking people's builds etc, so you might want to hold off on the commit for now.
Nov 8 2018
Could you add a test for --radix + --print-file-name, please, as the interaction could have changed with your implementation too. Otherwise looks good.
Nov 7 2018
I have no objection to INT32_MIN, as it's clear as to the purpose.
Not sure if you're referring to the original problem, but I assume you are. The original issue was highlighting a bug in MSVC's C++11 compatibility, as the type of the (now replaced) integer literal should be a signed long long automatically (effectively an int64_t), but instead it became an unsigned int. Casting it to an int64_t explicitly would also have worked, I think.
Please also add the following aliases:
- --syms is also a GNU readelf option as an alias for --symbols.
- --dyn-syms is the GNU readelf version of --dyn-symbols.
- --histogram is the GNU readelf long-form of -I/--elf-hash-histogram.
Great, LGTM, but please get somebody else to sign off too.
I'm honestly not really convinced that -0x80000000LL is more readable. I think it's a matter of personal taste. I think numeric_limits<int32_t>::min() is more explicit about what it's aiming to achieve. That being said, I really don't care that much, so if somebody else gives a LGTM, I don't mind.
Nov 6 2018
Wow, that's some spooky timing. I didn't realise somebody else was thinking the exact same thing as me at the time! I brought this up on the mailing list earlier today, not realising that this review existed!
I've got a few more nits, but otherwise I'm happy with this. Somebody else should definitely take a look too though.
Nov 5 2018
I haven't looked at all of this yet, by a long way, but here are a few comments on the first few files.
Nov 1 2018
LGTM, although please make sure that the whitespace is tidied up a bit, to make the indentation consistent, and reduce the number of long lines.
Might want to wait to see if @ruiu or anybody else has any other comments though.
LGTM, with the typo fix.
Nice spot. I obviously slipped a few times when reviewing! LGTM.
It would be good to see some doxygen comments in the header files.
I tried, and I still ended up with an STT_OBJECT...! But it doesn't matter at this point.
Oct 31 2018
I haven't even been able to get the assembler to emit a STT_COMMON symbol. What does the assembly and command-line look like?
What you did is fine for now. I do think we should consider working on the command-line options a bit more in llvm-objdump. There are a few issues that I know of already, including https://bugs.llvm.org/show_bug.cgi?id=37895 (which covers the missing single-letter alias options in the help text) and https://bugs.llvm.org/show_bug.cgi?id=31679 (which points out that single-letter options don't combine properly (e.g. you can't do -fsr). The command-line options are also not GNU-compatible in some instances, in that they do different things. I'm going to raise a thread on the mailing list about this, if I get a chance, as to fix it, we'd have to change the meaning of some of the current options.
Oct 30 2018
Great, thanks. Do you need me to commit it for you?
Just doing this to ask for the test update I mentioned.
On STT_COMMON, it looks like it only matters in fully linked files, since STT_COMMON symbols must always be in SHN_COMMON in relocatable objects. However, STT_COMMON can affect dynamic link-time semantics too, so I guess we may have to check for both. You should check GNU objcopy's behaviour in this case too (i.e. does an STT_COMMON symbol in a fully-linked ELF get affected or not by this option - I assume it is affected), and test accordingly. At the moment, the tests only test for the relocatable output case (i.e. where STT_COMMON is in SHN_COMMON), and we need cases for STT_COMMON not in SHN_COMMON.
Oct 29 2018
Use numeric_limits instead of literal.
Huh. I'd be interested to know WHY making a common symbol local is causing that effect, but I'm happy with this going in to fix it. You might want to prod @jakehehrlich though to be sure.
LGTM with the suggested changes in the CHECK command names.
http://lab.llvm.org:8011/builders/lld-x86_64-win7/builds/28829 is an example build showing the warning.
Do you need help committing this again? If so, could you confirm how I should write your name in the attribution, please? (e.g. Xing Guo/Guo Xing etc)
LGTM, with the nit.
Code change is fine, but please add a test to show that an unknown value is rejected with a useful error message.
I can certainly understand your reasoning, but this could also adversely impact a desired use-case of hiding a symbol in an object or library from another object, which is presumably the purpose of --localize-hidden, and is what is discussed in https://chromium.googlesource.com/external/dynamorio/+/master/core/CMakeLists.txt#541:
My feeling is that a single separate test is not the right way to test this. I think it makes more sense for an undefined symbol to be added to globalize.test and --keep-global-symbols.test. What do you think?
Sorry for the delay, I was off sick. I will be committing this shortly.
Oct 24 2018
Do you need me to commit it?