LGTM with the inline comment fixed.

LGTM, if you make the suggested test changes.

Ping!

Still a few comments to address, but this is mostly okay now.

Ping!

In D52180#1237257, @jakehehrlich wrote:

Who put me in charge of this? I'm clearly unqualified.

In D52023#1235082, @jhenderson wrote:

I've had to back out rL342233, because the LLD cache test started failing intermittently, and I don't know why (it works for me locally). Would somebody mind trying to apply this patch and see if they get a failure?

Looks like the test failure was due to a flaky test, so I've relanded this as rL342366.

Thanks for the patch! Just one comment to add: please make sure with any llvm-objcopy changes to include myself as a reviewer, and probably also @jakehehrlich as he's the code owner.

You've forgotten to include context in the latest diff. I can't review some aspects of this change without it.

In D51992#1235574, @inglorion wrote:

Thanks! For the case where you don't want the files to expire, can you instead set the expiration age to 0? That should stop the pruner from removing them.

Unfortunately, it's not possible to disable expiration age explicitly using llvm-lto. The interface it uses ignores attempts to set any of the cache options to 0, thus making it impossible to disable them completely if they are already set (and the expiration age is set by default). I don't know why the interface is this way, but it was a deliberate decision made some time ago.

I've had to back out rL342233, because the LLD cache test started failing intermittently, and I don't know why (it works for me locally). Would somebody mind trying to apply this patch and see if they get a failure?

I may have missed this somewhere, but what happens if you specify --decompress/--compress-debug-sections on a system without zlib?

Please remember to include me as a reviewer on all llvm-objcopy reviews.

Unfortunately, I'm going to be on annual leave as of next week, so I won't be in a position to review anything further until a week on Monday. As long as my outstanding points are addressed, I don't mind somebody else giving approval. I'll take a look at the final version when I get back, and can always request post-commit changes. Just make sure to include the "Differential Revision" part in the commit message so that this diff gets automatically updated with the committed version.

I'm liking the look of this now. Still got a few minor things still to work on though.

Okay, LGTM.

I'm thinking a bit more about this and I'm concerned that by adding these functions to Error.h, people will be more likely to use them from libraries, which can lead to various issues when the client of the library doesn't want to report errors and warnings the same way as this implements. The warn function was really only intended as a default implementation so that we didn't have to propagate things all the way out of DWARFContext.cpp, for the single client (llvm-dwarfdump) to consume.

@jakehehrlich, what are your thoughts on @alexshap's comments re. compress only when it makes things smaller?

In D51033#1207437, @jhenderson wrote:

I'm not convinced "Display" is the right term for the message.

This should say "for the function name"

I'm not convinced "Display" is the right term for the message. "Print" or "Dump" would be much more typical for reporting messages.

Shouldn't this have a test?

LGTM, from my point of view now.

I've just noticed that your .debug_str section appears to have SHF_MERGE and SHF_STRINGS set in the input, but the output does not have these flags set. Is this correct? Does GNU objcopy strip the flags? I doubt that it should...

This LGTM. But maybe worth giving it a day or two to see if anybody responds to the email thread. Or you could commit it as is, and then fix things if people complain in the thread, I don't mind.

If I get a chance, I'll verify this on Windows. Otherwise, this LGTM.

LGTM, with nit.

Hi @vleschuk, I just wanted to quickly check-in on this to see if you need any help with making further improvements as suggested?

LGTM.

In D50761#1200428, @grimar wrote:

After one more look, since the field name is SectionOrType, I think it is ok to accept decimals here too for specifying a section index.
It will also simplify the implementation a bit. I'll update the patch.

Oops, hang on, didn't see your change as I wrote my review...

LGTM, with nit. I guess if somebody wants decimal support later, they can add it then.

Is it normal for yaml2obj to only parse hex values and not decimal for other fields?

Could I make one request going forwards. It's helpful if you post a quick summary of the changes you made each time you update the diff (a bullet-point list would be fine). Due to the scale of the change, I'd prefer not to re-review everything each time I look, and sometimes it's a little hard to remember whether a bit has changed.

In D49678#1199294, @plotfi wrote:

In D50589#1199877, @rupprecht wrote:

I basically ran --globalize-symbol a --keep-global-symbol b for a/b being local, weak, and global. (I added this as a separate test case).
For GNU, b ended up global in the output file. But a was kept local unless it was originally local; when a was local in the input, it ended up being global in the output.

For reference, the GNU business logic is here: https://github.com/redox-os/binutils-gdb/blob/a9d73bb6dacfbbdc826fea178e3a47bf7a4154b1/binutils/objcopy.c#L1552
It seems that:

• --globalize intentionally only applies to local symbols, so we're already broken w.r.t gnu compatability. (I don't know what the history of that is in llvm-objcopy -- I can fix that if you want). So running GNU objcopy --globalize Weak1 --keep-global-symbol Weak2 will result in Weak1 being local.
• --globalize omits already global symbols (i.e. (flags & BSF_LOCAL)), but it only checks on the *original* binding type (i.e. it does not look at sym->flags). So --globalize G1 --keep-global-symbol G2 means that G1 will go from global->local (because it isn't included in --keep-global-symbol), but won't go back from local->global because the original binding was already global.
  
  I'm not sure if the weak symbol binding is an issue; I can fix that in its own commit if you want. For the global symbol binding issue, IMHO, that's a bug, and we should do the functionality you suggested: --globalize foo should keep foo global, whether or not --keep-global-symbol covers it. OTOH, it's also an edge case that probably never comes up.

I can't remember whether we deliberately decided to deviate from GNU on the --globalize behaviour. To me, our behaviour makes sense: if a user deliberately requests to globalize a weak symbol, why shouldn't we? I could even see a use-case for it: a member, in an archive, with a weak symbol definition, won't normally be used, unless other symbols from the archive are needed, so --globalize could force its inclusion. I'd go back and dig out the change that introduced the behaviour and check if it was discussed previously, and if not, file a bug or send an email on the mailing list to discuss it further.

LGTM, but there is one thing still outstanding, I think, which @jakehehrlich mentioned in a comment regarding the CopyConfig options:

Please make sure to rebase this patch when you get a chance, because at the moment, it's a little hard to apply the patch on trunk.

No null symbol is a blocker, so I've marked this as requesting changes. Sorry!

In D50589#1197922, @rupprecht wrote:

In D50589#1196944, @jhenderson wrote:

What happens if you specify a mixture of --globalize-symbol and --keep-global-symbol?

The symbol will be a global in the output file.
That's how it accidentally was in the way I wrote it, but I added a test case to make sure it stays that way and left some more comments.

I think I wasn't explicit enough with my wording to explain what I was thinking: what happens if you specify --globalize-symbol a --keep-global-symbol b? It looks like the code will turn a into STB_LOCAL, and b will remain as it currently is (similar points about --localize|weaken-symbol also apply). Does this match GNU's behaviour? I'd expect a to be STB_GLOBAL and b to remain unchanged.

What happens if you specify a mixture of --globalize-symbol and --keep-global-symbol?

I've run out of time of review the rest of this change today, but I will get back to it next week. Please could you try to fix the issues I've highlighted in the meantime.

In D50463#1194301, @jakehehrlich wrote:

I don't think I want this option to do anything more than literally add a prefix to all section names. It isn't clear that those precise semantics are being relied on anywhere and the behavior of making relocation names and not renaming symtab etc... is a result of how bfd reconstructs the output binary without consideration for what the user initially put in which is opposite the stated goal of llvm-objcopy.

I can't think of any semantic reason why names should matter for the cases discussed, with the exception of a tiny number of sections (usually to do with debug information or similar such things), names of sections shouldn't matter (and indeed, this has been discussed on the ELF gABI mailing list as a stated goal of ELF). This option WILL break DWARF sections though, which rely on the section naming. I'm not sure if this is something we should care about though?

In D50343#1194094, @rupprecht wrote:

Thanks for the review!

Alex/James/anyone else who's lurking, mind taking a look for a second set of eyes?

In D50463#1193520, @jhenderson wrote:

I've got some comments that I'll send through a bit later today, but as things stand, this doesn't match GNU objcopy's behaviour for relocation sections at the very least - the "prefix" is placed between the end of ".rela" and the start of the section name so that they have the name ".rela<prefixed relocated section name>"

It turns out that this is a wider bug in llvm-objcopy that is related to, but not specific to this issue: if you do --rename-section in GNU objcopy, you also implicitly rename relocation sections. Meanwhile, if you do --rename-section on relocation sections or strtab or symtab sections, nothing happens (if less concerned about this, because you requested something specific for those sections, but still, it is a behaviour difference).

I've got some comments that I'll send through a bit later today, but as things stand, this doesn't match GNU objcopy's behaviour for relocation sections at the very least - the "prefix" is placed between the end of ".rela" and the start of the section name so that they have the name ".rela<prefixed relocated section name>"

LGTM with one nit.

LGTM, with the suggested changes, but please get @jakehehrlich to confirm he's happy with the test changes.

I just noticed that llvmObject has an object_error equivalent to std::errc, with a make_error_code method allowing us to turn them into std::error_code for use with the new function. In particular, there are unexpected_eof and parser_failed errors, which might be more appropriate for certain situations in this file. What do you think?

Would you mind when you mark things as done, to upload the latest diff, as it makes it a little confusing for me seeing something marked as "done" when it doesn't appear to have been yet (might be best to then hold off marking off things as done until you are ready to upload).

@alexshap, are you happy for me to forward the email thread we had about refactoring llvm-objcopy to @rupprecht?

In D49979#1184977, @jakehehrlich wrote:

Due to the soon encroaching change where -I binary will be implemented I'm very much in favor of the OriginalData field being added to SectionBase. Really seems to handle everything very cleanly IMO.

Okay, go for it. I have nerves around lifetime issues with references like that, but I think the structure means that the Reader (and therefore the file it reads) will be around long enough for it to not matter.

I think @jakehehrlich's proposal more or less matches my thoughts. But I'm wondering why we need the OriginalData field? Do we have anything else that can modify the contents of a debug section? If not, why do we need a different field here? I may be missing something obviously...

In D49979#1183640, @jakehehrlich wrote:

So here's an idea. It is ELF specific but it extends the SectionBase interface in a way I've considered for a while and would make both this and another change quite easy. If we add an OriginalData field (ArrayRef<uint8_t>) to SectionBase we can dump that inside of HandleArgs.

In D49676#1183890, @vleschuk wrote:

Closed by rL338447

In D49870#1182950, @rupprecht wrote:

In D49870#1181835, @jhenderson wrote:

I don't really have an immediate preference, so I just added those two to the preserve mask -- let me know if I should add others.

I think this is a reasonable approach for now. I can't think of any situation where converting between TLS and non-TLS (and vice versa) makes sense, since the relocations needed are different.

In D49676#1182530, @vleschuk wrote:

• Fixed spacing in tests

Related aside to my comments regarding version mismatches: is it likely that future versions of DWARF (e.g. DWARF 6) will update the version number of the .debug_addr tables without changing anything in them? I know in previous versions of DWARF, some sections didn't increase in version number in line with the overall standard version. If not, then the version mismatch check may not make sense anyway.

Okay, I think I'm happy with this, but please let the others give their feedback and thumbs up too.

I'm not seeing anything that checks that the requested compression style is a supported style. I'd expect an error to be generated if a nonsensical version is specified.

The code looks correct to me (barring a couple of nits) for what you are trying to do, but I do have a couple of outstanding concerns, that I'd like to discuss further: