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).

In D55298#1321288, @sidneym wrote:

Full context? I use git format-patch -1 and upload the file it produces.

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.

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.

LGTM.

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.

LGTM.

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.

In D54939#1314030, @mstorsjo wrote:

In D54939#1314029, @alexshap wrote:

i will get to this diff early next week, sorry about the delay. A small side note - probably the simple changes to include/llvm/Object/COFF.h can be factored out from this diff and reviewed separately (and probably much faster).

Ok - splitting them out isn't hard, but I dunno what to make as test for them, if they are going in as such.

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?

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?

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).

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.

In D54422#1296590, @ruiu wrote:

I don't think this is necessarily a bug. At least, "predicating" the name of an output section does not seems a good idea to me. It is getting too tricky, and I don't like to add more complexity here. I generally do not encourage users use linker scripts as it makes linking slower and trickier, and it is to me an acceptable consequence that ICF folds input sections before linker scripts bin input sections to output sections.

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.

LGTM.

Sorry for not getting to this earlier. It's been a busy week for me.

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.

LGTM.

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.

LGTM.

Could you add a test for --radix + --print-file-name, please, as the interaction could have changed with your implementation too. Otherwise looks good.

In D54200#1290042, @ruiu wrote:

My preference is INT32_MIN which works both in C and C++, but I don't think 0x80000000 is more readable than numeric_limits<int32_t>::min(). Honestly it looks a bit more like an exercise to see if you really understand how negative numbers are represented in the two's complement, I think.

I have no objection to INT32_MIN, as it's clear as to the purpose.

In D54200#1289971, @arichardson wrote:

Is the problem here that we are missing a cast to int64_t?

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.

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.

I haven't looked at all of this yet, by a long way, but here are a few comments on the first few files.

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.

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.

In D53782#1282586, @rupprecht wrote:

In D53782#1282384, @jhenderson wrote:

LGTM.

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?

You need to pass in --elf-stt-common=yes, and AFAICT also need to be using GNU as (not sure if clang can produce STT_COMMON like this).

I tried, and I still ended up with an STT_OBJECT...! But it doesn't matter at this point.

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?

LGTM too.

In D53913#1281632, @ruiu wrote:

I do not see any problem of adding these flags for the sake of compatibility with GNU. What I wondered is whether or not you really have a need to make it nondeterministic. I thought that we might be able to just ignore these flags.

In D53804#1281883, @Higuoxing wrote:

Is this version ok? Shall I update other options flag in another patch? Thanks a lot for your advice

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.

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.

In D53803#1279991, @eush wrote:

I would suggest renaming the test to symbol-table-elf.test for consistency with other *-elf tests in this directory.

LGTM.

In D53821#1279012, @sfertile wrote:

Thanks for cleaning this up. LGTM.

We could also fix this by using INT32_MIN or std::numeric_limts<int32_t>::min(), if preferred

Personally I prefer either of those over the raw numeric literal.

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.

In D53576#1274407, @Higuoxing wrote:

Updated with clang-formatted codes, Please help me commit it, thanks a lot :)

Do you need me to commit it?

In D53576#1274235, @Higuoxing wrote:

Hi, I upload a new test. Did I miss something? Thanks a lot