The rest of the patches in this series have been approved. It's just this patch that needs looking at. There's no particular need to look at most of the files that were in the debuginfo-test folder, as they've simply been moved. I'd recommend reviewing the changes outside that folder, plus the CMake files.

Please add the new option to the llvm-readobj documentation.

Not had time to review the test cases yet, but will do that next week.

This isn't an issue for the static linking approach, so why does it happen for the dynamic library approach? It seems like fixing this aspect should be the way forward, as it will be more robust generally.

Okay, no more comments from me at this time. Someone with GOFF knowledge needs to review the functionality for correctness.

I take it the new output is just what the head of LLVM produces when rerunning the examples? If so, LGTM.

Are call graph sections expected in fully linked output?

LGTM.

In other places, we've tested large obejcts by using a compressed input, and then extracted it at test time, although the examples I see are only a few megabytes in size, rather than gigabytes. I'm not convinced we want such such a large object lying around, so looks good to me.

LGTM too. Please remember to update the test once the string table printing has landed.

Ping #4!

Just trying to follow the logic through. I think there is a potential behaviour change, which may be fine, but I just want to highlight it to see what others think (I have no use for IHEX myself, so don't feel like I can make a completely fair judgment either way). If I follow it correctly, if a writable section were not in a PT_LOAD segment, then previously it was skipped, if there was at least one writable section that was in such a segment, whereas now it won't be skipped. This is probably harmless (all such sections should have been placed in a segment anyway). Assuming I'm right, should we have a test-case to illustrate this behaviour change?

Maybe -C/-A/-B are available to match grep because the current option is quite long?

Fix missed clang-format issue, and unchecked Exception (caused by broken check).

LGTM, with nit.

The size went up from 107KB to 322KB, aggregate of all the input sections.

LGTM. I'm not in a good position to push this myself right now, due to some other pending work. @evgeny777/@MaskRay (or anybody else), could you push this for @mciantyre, please?

In D103490#2806504, @yusra.syeda wrote:

ping :)

In D103455#2805561, @Esme wrote:

Addressed comments except dumping the raw string table.
It seems llvm-readobj can't dump the raw string table for XCOFF, I have to find another tool to do this.

Ping!!

Lots of stylistic comments from me. Someone else will need to review functionality.

Test suggestion looks reasonable to me, although I might suggest one slight tweak, namely to use --implicit-check-not rather than CEHCK-NOT after the symbol check. In the latter case, there's nothing stopping the symbol appearing before the set that you are checking.

LGTM. It looks like there are non-standard line endings in this file - please fix them up whislt your're at it (either in the same or another commit).

LGTM too. I've got a lot else on my plate, so if @dblaikie or @MaskRay are able to land this, that's preferable. Otherwise, I'll try to do so in the next couple of days.

In D103546#2799634, @vzakhari wrote:

In D103546#2799569, @MaskRay wrote:

We have lib/BinaryFormat/Magic.cpp. Do we need the ELF specific API?

Thanks! I was not aware of this code. How about adding something like static inline is_any_elf(file_magic) method in Magic.h? If you agree, then I will abandon this change-set and add the new method in D103545.

Mostly looks good to me, but would be nice if one of the earlier reviewers gives thumbs up before we commit this patch.

Could you please split this patch into a patch that adds the new enum values, which would include the llvm-readobj and yaml2obj/obj2yaml changes, and then another patch (or patches) with the rest in? The latter builds on the former, but the former doesn't need the latter to work.

Looks good again. Sorry for the delay - I was off work last week.

Sorry for the delay - I was off work last week. Some minor comments, otherwise basically this is good.

In D103156#2787777, @kwk wrote:

@jhenderson @thopre it works for me now! Thank you!

@kwk, could you try again please with the latest version of the patch? The code now looks at the same paths as are added to the PATH environment variable, so it should work.

Use the right set of paths in use_clang. This should fix the issue. If it doesn't I'll need someone who can reproduce to dig more into why it doesn't.

Ah, I see I missed that paths is reused in use_clang, before the call to use_llvm_tool. I'll fix that now.

Rebase. I'll try landing this once the pre-merge checks complete, if there are no objections.

LGTM, with one spelling error. Thanks!

LGTM overall. Just a few nits.

LGTM, thanks!

In D103154#2784076, @jhenderson wrote:

Remove unrelated changes.

I'm still investigating the cause of the test failure - it doesn't fail for me locally on my windows machine.

Remove unrelated changes.

Upload correct patch.

Remove unrelated parts of diff.

Nearly there. Not approving since there's a build error though...

In D103156#2783447, @tstellar wrote:

D103154 seems to be making a lot of the same changes as this patch.

In D103154#2783705, @MaskRay wrote:

I just understand what D102630 and this do. Now rm $build/bin/ld.lld; llvm-lit something.s will not pick a ld.lld from PATH. Looks great!

curl -L 'https://reviews.llvm.org/D103154?download=1' | patch -p1
ninja check-lit

says use-llvm-tool.py fails.

In D103156#2781804, @teemperor wrote:

Depends on D101354.

Typo?

Fix another comment.

Fix some stale comments.

LGTM.

LGTM, thanks!

In D102277#2781401, @avl wrote:

I don't think it's a goal of llvm-objcopy to prevent users from using format-specific options when those formats are not present, or when other formats are present. GNU objcopy doesn't do this, so doing it ourselves is probably a bad idea, as it makes the tool less compatible with GNU, for no real benefit. The exception is for unimplemented options, as discussed earlier.

the problem is in how following case might be handled:

--add-symbol sym1=0x100,global --add-symbol sym2=0x200,elf-specific-flag --add-symbol sym3=0x300,coff-specific-flag

If we allow multiple files with different formats and allow format specific options then above legal situation
could not be handled: while parsing options, for elf there would be detected unknown "coff-specific" value,
for the coff there would be detected unknown "elf-specific" value. This would result in a error for both input files.
i.e. it looks like weak command-line interface which does not work for legal case.

From the other side, GNU objcopy does not solve this case also and the whole case looks quite specific.
so, if we are fine with such a behavior then let it be so.

LGTM from a yaml2obj standpoint, but it would be a good idea to rope in someone with XCOFF experience to confirm they are happy with the file format writing before committing this.

In D103125#2780936, @dblaikie wrote:

Can't say I'm super enthusiastic about this (I assume the build already supports prefixes and suffixes, which I'd hope would be adequate - but presumably are not for your use case), though there's some, I think, related prior art: Sony folks (@probinson @jhenderson) have (or had at some point) different C++ language standard/version defaults than upstream and have maintained/made changes to upstream test cases that assume the upstream default version to not make that assumption (to have it explicit). So having some costs/changes upstream for downstream differences like this seems at least vaguely plausible to me.

LGTM, with nit.

In D102296#2780324, @dblaikie wrote:

In D102296#2778940, @jhenderson wrote:

In D102296#2778439, @Amir wrote:

@jhenderson: thanks for the suggestion. I'm going to add the Object unit test for this change.
The motivation is the use of relocations by BOLT for function reordering optimization (specifically, to find references to functions). We recommend that users keep relocations in the final binary: https://github.com/facebookincubator/BOLT#usage

I think I follow now, thanks. Dynamic relocations should probably still be omitted by this lookup, i.e. getRelocatedSection only deals with static relocations. Not all dynamic relocation sections have an associated section, so this function will fail on them.

llvm-dwarfdump is relying on getRelocatedSection() to return section_end() for ELF files of types other than relocatable objects. We've changed the function to return relocatable section for other types of ELF files. As a result, llvm-dwarfdump started re-processing relocations for sections that already had relocations applied, e.g. in executable files, and this resulted in wrong values reported.

It seems to me that for Elf_Rel relocations, you can't really reapply them, because the addend has been overwritten by the final patched value. So, as I understand it, the only way you can reliably know how to reapply relocations is with Elf_Rela relocations. I think this means you need to change llvm-dwarfdump too, to only reapply relocations for ET_REL objects? (It's harmless to do so for other ET_* kinds, iff the relocation types are Elf_Rela of course, but it's pointless work).

Presumably we don't want to reapply them, right? (unless BOLT is modifying the relocations and expecting them to be reapplied) I'm assuming there's some property we should be able to observe that indicates the relocations have already been applied so we should ignore them?

In D101341#2755605, @vzakhari wrote:

In D101341#2753240, @jhenderson wrote:

@vzakhari, what's the motivation for doing this? These data structures are used within yaml2obj/obj2yaml, although I'm not particularly familiar with the plumbing to identify whether it matters. My suspicion is that it doesn't.

@jhenderson, there are two ways to create a YAML file using YAML mappings. The first one is to create a YAML file from a string bufffer (see line 38 in llvm/unittests/ObjectYAML/ELFYAMLTest.cpp). The second way is to populate the ELFYAML:: data structures and stream them into yaml::Output. Both ways are valid. The second way is a little bit more convenient for programmatic ELF-YAML creation in my downstream application. When I try to use ELFYAML:: data structures, there is no convenient way to make sure that all optional members in them are initialized. Neither default nor zero-initialization works. This is why I made these changes to make sure that all optional members are initialized properly. Note that the default values are not necessarily zeroes.

In D102277#2779104, @avl wrote:

I edited my previous comment with this part:

Actually, current format specific checks verify exactly this : "avoid using of format specific options when multiple inputs in different formats are specified". That probably means that we need to keep them? (To prevent incorrect lazy options parsing)

LGTM, thanks!

In D102277#2779096, @avl wrote:

Short answer: yes the tools are supposed to work with multiple formats in a single run. llvm-objcopy/llvm-strip need to be compatible with GNU objcopy/strip. I just ran a quick experiment and GNU objcopy is able to handle inputs with different file formats in the same run (I specifically tested COFF and ELF). As such, llvm-objcopy needs to be able to handle the same cases.

For reference, I have heard of instances in the past where people create a single archive containing files of different formats, so that they can use it in a cross-platform manner. The specific example I know of was for an ELF32 target and an ELF64 target, but I believe the principle is equally applicable for COFF, Mach-O etc.

I see. From previous conversation I had an impression that we do not expect multiple inputs in different formats(https://reviews.llvm.org/D99055#2701025). Also, There would be problems with parsing lazy options for the case "multiple inputs in different formats". From that point of view, restriction on avoiding usages of "multiple inputs in different formats" might make sense. Probably, useful restriction would be - avoid using of format specific options when multiple inputs in different formats are specified.

Anyway, if "multiple formats in a single run" is legal then it looks like we need to avoid format specific checks as you suggested. Will update the diff.

Taking the conversation out of line.