Sorry, for not getting to this patch sooner, I dropped it due to vacation + some unexpected things... I'm still eager to get this patch in :)

• Remove useless parens
• Rename inXXXSegment->isBerkeleyXXX
• Clarify segment comments
• Add test cases that ensure section flags are used instead of section names

Is there a motivation for this change or just cleanup?

In D54674#1325009, @plotfi wrote:

In D54674#1322482, @lhames wrote:

Apart from these specific comments, my general thought is that this is fine as far as it goes, but as it grows more fully featured it seems likely to start overlapping more libObject and MC functionality.

Have you considered ways to refactor libObject/MC to allow objcopy to be built on top of them?

It's been suggested before (https://lists.llvm.org/pipermail/llvm-dev/2018-October/126661.html), but no concrete proposals have been put forward AFAIK.
With this and D54939, llvm-objcopy is about to go from ELF to ELF+MachO+COFF. IMO, we shouldn't block either of these patches on that idea, but we should start moving bits from llvm-objcopy into libObject soon after landing both:

1. Landing these patches sooner will give people more time to play around with MachO/COFF support in the tool (ELF support is fairly well known)
2. Being able to refactor things once all three object formats are in will help motivate the design (i.e. won't be an ELF-centric design)

Thanks!

Just a nit on the naming; actual functionality/test LGTM.

In D54674#1303281, @alexshap wrote:

also, @jakehehrlich, @jhenderson, @rupprecht - what would you say to moving the existing tests (llvm/test/tools/llvm-objcopy) into the subfolder ELF (llvm/test/tools/llvm-objcopy/ELF) ? if it's ok - i can do that on a separate diff.

LGTM for the actual style change, just a wording nit:

cc'ing blaike as a local debug expert -- I'm not familiar with systems using --build-id

In D54124#1292583, @jhenderson wrote:

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.

Any thoughts on how to drum up some more interest?

Updated the description, thanks for pointing that out.

fix test flags

• Renaming/removing unnecessary externs/more test cases/other comments

• Add radix+filename test

• Use formatv

• Remove unnecessary size parameter

• Clean up aliases/tests

@djasper -- ping

• Only expose compatibility args (e.g. --relocs for --relocations) in readelf mode

• Hide -t as an alias for --symbols in readelf mode

In D54124#1288822, @jhenderson wrote:

Wow, that's some spooky timing.

Happy belated Halloween! :)

In D54118#1287750, @MaskRay wrote:

If the initial attempt to bring the -s incompatibility in llvm-readelf was inadvertent, let's improve it!

--help is getting to the point where it might be clean enough to write a test for :)

• Use better flag parsing, and include alias in --help

• Revert ET_EXEC/STT_COMMON from tests as we aren't doing that anymore

• Fix typos

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?

• Localize common symbols only if they are actually in SHN_COMMON

In D53913#1281963, @jhenderson wrote:

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.

Personally, I think that if the default is to make them deterministic, then I don't see any reason not to allow overriding this default, as the code does not become significantly more complex with this change. However, I could be persuaded that nobody has this use case.

• Implement last-one-wins for -D/-U

In D53913#1281610, @ruiu wrote:

It was before this patch and it still is. This is providing the option to make it not default (if anyone wants that), and also won't break if someone is explicitly running "objcopy --enable-deterministic-archives" and we drop llvm-objcopy in place of that.

Oh sorry I didn't notice that. What was the motivation for you to add a new flag so that you can choose deterministic and non-deterministic? (Just wondering.)

• Add test case that deterministic is the default

In D53913#1281601, @ruiu wrote:

I feel it should be enabled by default, as deterministic build is a very nice property.

In D53782#1280158, @jhenderson wrote:

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.

GNU objcopy seems to only ignore --localize options for symbols where the index is SHN_COMMON, but not when it is STT_COMMON in some other section.
Based on an earlier comment, I changed this to respect index == SHN_COMMON and type == STT_COMMON equally, i.e. --localize-* will not be applied in either of those cases. So this would be breaking compatibility. I don't have a preference one way or the other; I'm happy to change it back if someone feels strongly.

• Update test cases with common symbols not in SHN_COMMON
• Rebase after refactoring

Seconding the ping to commit -- I'm still regularly seeing buildbot failures for this test.

In D53733#1278600, @jhenderson wrote:

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?

• typo

• Move test cases to already existing tests

• Also check STT_COMMON