[llvm-objcopy] Add --strip-unneeded option

[paulsemel]

Summary

This option removes symbols that are not needed by relocations.

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

159

it's not a global - thus it won't be initialized by zero by default, might contain garbage. Am i missing smth ?

[paulsemel]

Initialized the option to false by default.

tools/llvm-objcopy/llvm-objcopy.cpp

159

Well.. just missed the initialization :/

[jakehehrlich]

tools/llvm-objcopy/Object.cpp

658–665

I don't nessarilly think this should be done in this change but I think I don't like how I wrote addRelocation. I think it should take the parameters to build a relocation not request that the user build the relocation. That would have the added benefit of allowing addRelocation to update the aforementioned count.

tools/llvm-objcopy/Object.h

347

I'm not sure this is sufficient, or if it is it has to be set at the right time. For example say a symbol is referenced by a relocation in .rel.data and then .rel.data is removed. It could be that that symbol is either still referenced by another relocation in another section or it could not be. No local operation would tell you the answer either.

I think there are two possibilities.

1. Maintain a count that can be incremented/decremented as relocations are added/removed
2. Do a pass at the end to determine which symbols should be kept/not kept. You can either set this bool in that pass or add symbols to a set like container. see: http://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc

379

I don't really like this method. It's really specific and most of the overly specific methods I've implemented in objcopy have needed to be generalized later or caused some sort of issue. In this case I think just adding a non-const version of getSymbolByIndex is sufficient and more general.

tools/llvm-objcopy/llvm-objcopy.cpp

392

The whole Sym.Index != 0 smells to me and is a product of me not thinking about the dummy symbol. At the very least that should never be past to a function in removeSymbols or updateSymbols. It might be better to handle the dummy symbol the way we handle the dummy section, namely not store it and account for the increased index. Probably best for another change though. Could you add a TODO here for that?

393

Did you observe that GNU objcopy only worked on ET_REL? Also did you observe GNU objcopy not use this for locals? I think it makes perfect sense to remove undefined globals/weaks not used in a relocation.

[jhenderson]

ET_REL is different to ET_DYN/EXEC because the former is usually the input to a static linker, whereas the latter isn't (at least, not in the same sense). The difference is the use of static relocations versus dynamic relocations. llvm-objcopy already knows about the difference between these relocation sections, by using whether SHF_ALLOC has been set, and as far as I remember, it doesn't pay attention to the elf type at all (except to reflect it in the output file). I don't see why we should pay any more attention to the type in this case either.

In summary: like @jakehehrlich, I don't expect to see the ELF type check, nor do I think we need different tests for the different types.

[paulsemel]

I'm sorry guys, but I think I am missing something. Indeed, if I used the ELF type and not something else, it's because objcopy is keeping global and weak symbols (except for undefined ones) for relocatable files.
This is actually done even if the symbol is not referenced by a relocation. That's why I don't really see what I can do with SHF_ALLOC.
Can you elaborate on this one ?

tools/llvm-objcopy/Object.cpp

658–665

Well, maybe it can be a bit confusing to do this "fix" in this change.. I can add a TODO if you prefere ?

tools/llvm-objcopy/Object.h

347

Arf.. I totally forgot the "what if the reloc section is removed" part Thanks about it !
I think I will go for the counter, as I first wanted to be as light as possible for this option (and I don't see anything that might cause problems with the counter).

379

Ok, sounds fair !

tools/llvm-objcopy/llvm-objcopy.cpp

392

I totally agree with you ! I Actually wanted to talk with you about this dummy symbol, because there is some problems, especially with sections where the only remaining symbol is the dummy one (we might want to remove the section).
I will add a TODO, no problem ! :)

393

You're right, I missed this too..
It actually removes global/weak symbols that are undefined and not used in relocation. I'm going to change this to fit the objcopy behavior, thanks for spotting this !

[jhenderson]

In D46896#1100897, @paulsemel wrote:

I'm sorry guys, but I think I am missing something. Indeed, if I used the ELF type and not something else, it's because objcopy is keeping global and weak symbols (except for undefined ones) for relocatable files.
This is actually done even if the symbol is not referenced by a relocation. That's why I don't really see what I can do with SHF_ALLOC.
Can you elaborate on this one ?

Sorry, I probably wasn't clear enough. You have a check for the object type in the change you are proposing. My point was that it is unnecessary. There shouldn't be a difference in llvm-objcopy's behaviour in this case. The code to mark a symbol as needed is only called for instances of RelocationSection, which deals with static relocations. Such relocations only appear typically in ET_REL inputs, but it should be harmless to modify them regardless of input type.

[paulsemel]

In D46896#1101079, @jhenderson wrote:

In D46896#1100897, @paulsemel wrote:

I'm sorry guys, but I think I am missing something. Indeed, if I used the ELF type and not something else, it's because objcopy is keeping global and weak symbols (except for undefined ones) for relocatable files.
This is actually done even if the symbol is not referenced by a relocation. That's why I don't really see what I can do with SHF_ALLOC.
Can you elaborate on this one ?

Sorry, I probably wasn't clear enough. You have a check for the object type in the change you are proposing. My point was that it is unnecessary. There shouldn't be a difference in llvm-objcopy's behaviour in this case. The code to mark a symbol as needed is only called for instances of RelocationSection, which deals with static relocations. Such relocations only appear typically in ET_REL inputs, but it should be harmless to modify them regardless of input type.

No it's ok :)
The fact is that objcopy cares about those "static relocations". Or at least it was the case when I tested for those ones.
So what you say is that we should just strip those symbols ? It's really weird, because it would lead to a broken relocatable file..

[jhenderson]

In D46896#1101102, @paulsemel wrote:

In D46896#1101079, @jhenderson wrote:

In D46896#1100897, @paulsemel wrote:

I'm sorry guys, but I think I am missing something. Indeed, if I used the ELF type and not something else, it's because objcopy is keeping global and weak symbols (except for undefined ones) for relocatable files.
This is actually done even if the symbol is not referenced by a relocation. That's why I don't really see what I can do with SHF_ALLOC.
Can you elaborate on this one ?

Sorry, I probably wasn't clear enough. You have a check for the object type in the change you are proposing. My point was that it is unnecessary. There shouldn't be a difference in llvm-objcopy's behaviour in this case. The code to mark a symbol as needed is only called for instances of RelocationSection, which deals with static relocations. Such relocations only appear typically in ET_REL inputs, but it should be harmless to modify them regardless of input type.

No it's ok :)
The fact is that objcopy cares about those "static relocations". Or at least it was the case when I tested for those ones.
So what you say is that we should just strip those symbols ? It's really weird, because it would lead to a broken relocatable file..

No, I didn't say that at all, sorry. --strip-unneeded should, by my understanding, remove symbols that are not the target of a static relocation (this is what you have implemented). I don't think you need to check the file type.

[paulsemel]

In D46896#1101231, @jhenderson wrote:

In D46896#1101102, @paulsemel wrote:

In D46896#1101079, @jhenderson wrote:

In D46896#1100897, @paulsemel wrote:

I'm sorry guys, but I think I am missing something. Indeed, if I used the ELF type and not something else, it's because objcopy is keeping global and weak symbols (except for undefined ones) for relocatable files.
This is actually done even if the symbol is not referenced by a relocation. That's why I don't really see what I can do with SHF_ALLOC.
Can you elaborate on this one ?

Sorry, I probably wasn't clear enough. You have a check for the object type in the change you are proposing. My point was that it is unnecessary. There shouldn't be a difference in llvm-objcopy's behaviour in this case. The code to mark a symbol as needed is only called for instances of RelocationSection, which deals with static relocations. Such relocations only appear typically in ET_REL inputs, but it should be harmless to modify them regardless of input type.

No it's ok :)
The fact is that objcopy cares about those "static relocations". Or at least it was the case when I tested for those ones.
So what you say is that we should just strip those symbols ? It's really weird, because it would lead to a broken relocatable file..

No, I didn't say that at all, sorry. --strip-unneeded should, by my understanding, remove symbols that are not the target of a static relocation (this is what you have implemented). I don't think you need to check the file type.

Ok, so for this one, we do not follow objcopy behavior ?

[jhenderson]

I'm now getting a bit lost. Please could you outline the GNU objcopy behaviour for the following axes:

Axis 1: elf type (ET_REL versus ET_DYN/ET_EXEC)
Axis 2: symbol binding (Global/Weak/Local)
Axis 3: undefined/defined symbols

This should help us evaluate the behaviour and determine if the approach taken is correct. It should be straightforward to generate these different cases using yaml2obj, like you do in the tests.

[paulsemel]

In D46896#1101273, @jhenderson wrote:

I'm now getting a bit lost. Please could you outline the GNU objcopy behaviour for the following axes:

Sure, I'll try to be as much consistent as I can.

Axis 1: elf type (ET_REL versus ET_DYN/ET_EXEC)
Axis 2: symbol binding (Global/Weak/Local)

There is a real thing here. The fact is that, as you already know, for ET_REL, global symbols might be link with other relocatable files. For this reason, and only for ET_REL, objcopy is keeping those symbols.
I will try to give you an example. Consider the test file test/tools/llvm-objcopy/localize.test. If you yaml2obj it, and launch objcopy --strip-unneeded %t %t1, you will notice that those the weak and global symbols are kept in the result binary, despite the fact there are not referenced in a relocation.
Now, if you take this same test file but change from ET_REL to ET_DYN (and do the same procedure again), you will notice that the symbols are not present anymore (symtab should be removed actually).
This is what I'm trying to reproduce with this behavior. The fact is that, if you take a look at objcopy code, you won't see anything about this ET_REL handling.
But the fact is that they are also handling this option in the section removal part of objcopy, which makes them removing symbol table when this option is set.

I didn't do it this way because I truly think that the symtab removal might be done in the writing part of llvm-objcopy. By this I mean that we might test whether the symtab is empty, and if so, just remove it. The second thing is that I wanted to avoid the problem mentioned by @alexshap in an other review with the keep-symbol option. This way of doing, we are avoiding the problem.

To summarize: For ELF other than ET_REL, objcopy is basically stripping everything (except if in a reloc). For ET_REL, it tries not to leave the binary in a broken state. Indeed, if we also remove Global/Weak symbols, we are just breaking the linking part (that's not what we want).

Axis 3: undefined/defined symbols

For other than ET_REL, objcopy doesn't seem to care about undefined/defined symbols. BUT, as @jakehehrlich mentioned, undefined Global/Weak symbols (again, that are not present in reloc) are stripped from the binary. This is, as far as I can tell, the only time objcopy strips G/W symbols in ET_REL Elf files.

This should help us evaluate the behaviour and determine if the approach taken is correct. It should be straightforward to generate these different cases using yaml2obj, like you do in the tests.

Hope this clarifies, please tell me if you want me to elaborate on some other points :)

[jakehehrlich]

In D46896#1101378, @paulsemel wrote:

In D46896#1101273, @jhenderson wrote:

I'm now getting a bit lost. Please could you outline the GNU objcopy behaviour for the following axes:

Sure, I'll try to be as much consistent as I can.

Axis 1: elf type (ET_REL versus ET_DYN/ET_EXEC)
Axis 2: symbol binding (Global/Weak/Local)

There is a real thing here. The fact is that, as you already know, for ET_REL, global symbols might be link with other relocatable files. For this reason, and only for ET_REL, objcopy is keeping those symbols.
I will try to give you an example. Consider the test file test/tools/llvm-objcopy/localize.test. If you yaml2obj it, and launch objcopy --strip-unneeded %t %t1, you will notice that those the weak and global symbols are kept in the result binary, despite the fact there are not referenced in a relocation.
Now, if you take this same test file but change from ET_REL to ET_DYN (and do the same procedure again), you will notice that the symbols are not present anymore (symtab should be removed actually).
This is what I'm trying to reproduce with this behavior. The fact is that, if you take a look at objcopy code, you won't see anything about this ET_REL handling.
But the fact is that they are also handling this option in the section removal part of objcopy, which makes them removing symbol table when this option is set.

I didn't do it this way because I truly think that the symtab removal might be done in the writing part of llvm-objcopy. By this I mean that we might test whether the symtab is empty, and if so, just remove it. The second thing is that I wanted to avoid the problem mentioned by @alexshap in an other review with the keep-symbol option. This way of doing, we are avoiding the problem.

To summarize: For ELF other than ET_REL, objcopy is basically stripping everything (except if in a reloc). For ET_REL, it tries not to leave the binary in a broken state. Indeed, if we also remove Global/Weak symbols, we are just breaking the linking part (that's not what we want).

Axis 3: undefined/defined symbols

For other than ET_REL, objcopy doesn't seem to care about undefined/defined symbols. BUT, as @jakehehrlich mentioned, undefined Global/Weak symbols (again, that are not present in reloc) are stripped from the binary. This is, as far as I can tell, the only time objcopy strips G/W symbols in ET_REL Elf files.

This should help us evaluate the behaviour and determine if the approach taken is correct. It should be straightforward to generate these different cases using yaml2obj, like you do in the tests.

Hope this clarifies, please tell me if you want me to elaborate on some other points :)

Eh I think this is kind of obscure behavior for GNU objcopy. I don't think it matters what we choose to do with symbols for linked binaries. For ET_REL we definitely need to preserve any symbol referenced by a relocation and we definitely need to preserve defined global/weak symbols. All other symbols should be removed. I'm fine being more aggressive for none ET_REL but I also don't care if we aren't. It's fine if you match the behavior for ET_EXEC and ET_DYN and its fine if you don't. My preference is for simpler code with fewer edge cases.

tools/llvm-objcopy/Object.cpp

658–665

That's fine.

tools/llvm-objcopy/Object.h

347

I was thinking about this and I was wondering if this isn't something that should be more general. Like it there are other ways symbols might be needed other than relocations. This should be a generic reference count. But we already have shared_ptr and weak_ptr to tell us how this should work!

Can we think of a way to reuse that infrastructure? For instance those types already have use_count methods so that can be used to determine weather a symbol is needed or not. Alternatively it would make sense to create a similar sort of abstraction that managed the reference counting for us.

tools/llvm-objcopy/llvm-objcopy.cpp

392

We probably don't want to remove empty symbol tables IMO. I don't think it has any real tendency to come up for any practical purpose where the desire wouldn't just be to remove the section directly.

[jhenderson]

I'm happy to defer to @jakehehrlich's preferences here, but FWIW, I think GNU objcopy's behaviour is actually the wrong way around regarding the Global/Weak symbols: if you strip a defined symbol from an ET_REL ELF that isn't referenced within that ELF, the file can still be used in a link, you just can't reference anything that wasn't already referenced by that object file. Stripping symbols from ET_DYN/EXEC is okay, as it should have zero impact on the runtime behaviour (although debugging etc might not be as trivial). Anything that requires symbols at runtime should be using dynamic relocations and dynamic symbols, which this behaviour won't touch.

I think we need to be slightly wary of using ET_* types directly, because it will make it hard for vendors to add their own ET_* types to the list of things that should/should not be affected by some behaviour. If we do decide to go down the route of matching objcopy's behaviour here and therefore need to distinguish between relocatable and executable types, I'd suggest adding helper functions to wrap them, and call those instead of testing e_type directly.

[paulsemel]

Hi !

I kind of disagree with one point. If we chose to not differentiate between ET_* files, we need to keep Global/Weak defined symbol in every cases.
Indeed, it just doesn't make sense to strip thoses symbols for ET_REL files, as it will make the relocatable file not usable.. (or kind of not usable).
I actually don't care for the remaining part (even if I admit I like objcopy current behavior).
I will wait for you to tell me what to do, I don't want to take decisions you will not approve :)

tools/llvm-objcopy/Object.h

347

Hm.. ok do you already have something in mind ?
The fact is that not only relocation can reference symbols, so we need to be able to distinguish between blocking references and normal references if you know what I mean.

tools/llvm-objcopy/llvm-objcopy.cpp

392

I don't get it. What's the point of keeping an empty symtab ?

[paulsemel]

Applied some suggestions.
For the moment, a counter has been implemented for the references.. waiting for some suggestions to change this.
Same, I let the ET_* things, waiting for some thoughts about it.

[jakehehrlich]

I'll be back to respond further. I'm pressed for time today.b

tools/llvm-objcopy/Object.h

347

Right which seems to be like a shared_ptr vs weak_ptr difference. I thought a tiny bit more about it and we don't want to connect collection to this. So I think I'd actually prefer an abstraction that has an explicit "clone" method that wraps pointers without calling delete on them. It seems like most ways of doing this I can think of require an extra layer of indirection. If you want we do explicit reference counting and I'll try and come up with something better later. Basically I think manual reference counting is error prone and I'd like to avoid it.

[jakehehrlich]

In D46896#1102723, @jhenderson wrote:

I'm happy to defer to @jakehehrlich's preferences here, but FWIW, I think GNU objcopy's behaviour is actually the wrong way around regarding the Global/Weak symbols: if you strip a defined symbol from an ET_REL ELF that isn't referenced within that ELF, the file can still be used in a link, you just can't reference anything that wasn't already referenced by that object file. Stripping symbols from ET_DYN/EXEC is okay, as it should have zero impact on the runtime behaviour (although debugging etc might not be as trivial). Anything that requires symbols at runtime should be using dynamic relocations and dynamic symbols, which this behaviour won't touch.

I think we need to be slightly wary of using ET_* types directly, because it will make it hard for vendors to add their own ET_* types to the list of things that should/should not be affected by some behaviour. If we do decide to go down the route of matching objcopy's behaviour here and therefore need to distinguish between relocatable and executable types, I'd suggest adding helper functions to wrap them, and call those instead of testing e_type directly.

First off, I think I agree with you on ET_* and the more I think about it the more I agree. I think given a) You're preference for not using ET_ and b) my initial and now growing preference we should not use ET_. @paulsemel please do not use ET_* checks in this way (if at all really)

Second. I think I disagree with removing defined symbols using --strip-uneeded. I interpret "uneeded" as "100% not needed in any possible link" not "uneeded to fully link this". For instance a library might define lots of functions that it doesn't call and are only meant to be used by other bits of code. Such symbols are quite common and shouldn't be removed by --strip-uneeded IMO. I'm happy for --strip-uneeded-symbol to behave the way you describe however.

[paulsemel]

In D46896#1104127, @jakehehrlich wrote:

In D46896#1102723, @jhenderson wrote:

I'm happy to defer to @jakehehrlich's preferences here, but FWIW, I think GNU objcopy's behaviour is actually the wrong way around regarding the Global/Weak symbols: if you strip a defined symbol from an ET_REL ELF that isn't referenced within that ELF, the file can still be used in a link, you just can't reference anything that wasn't already referenced by that object file. Stripping symbols from ET_DYN/EXEC is okay, as it should have zero impact on the runtime behaviour (although debugging etc might not be as trivial). Anything that requires symbols at runtime should be using dynamic relocations and dynamic symbols, which this behaviour won't touch.

I think we need to be slightly wary of using ET_* types directly, because it will make it hard for vendors to add their own ET_* types to the list of things that should/should not be affected by some behaviour. If we do decide to go down the route of matching objcopy's behaviour here and therefore need to distinguish between relocatable and executable types, I'd suggest adding helper functions to wrap them, and call those instead of testing e_type directly.

First off, I think I agree with you on ET_* and the more I think about it the more I agree. I think given a) You're preference for not using ET_ and b) my initial and now growing preference we should not use ET_. @paulsemel please do not use ET_* checks in this way (if at all really)

Second. I think I disagree with removing defined symbols using --strip-uneeded. I interpret "uneeded" as "100% not needed in any possible link" not "uneeded to fully link this". For instance a library might define lots of functions that it doesn't call and are only meant to be used by other bits of code. Such symbols are quite common and shouldn't be removed by --strip-uneeded IMO. I'm happy for --strip-uneeded-symbol to behave the way you describe however.

Ok, so we should keep defined symbols in any way, regardless the ELF type ? That's sounds really weird for me, as there is plenty of way we want to remove them (especially in ET_EXEC), but we can go for it :)

[jhenderson]

In D46896#1104130, @paulsemel wrote:

In D46896#1104127, @jakehehrlich wrote:

In D46896#1102723, @jhenderson wrote:

I'm happy to defer to @jakehehrlich's preferences here, but FWIW, I think GNU objcopy's behaviour is actually the wrong way around regarding the Global/Weak symbols: if you strip a defined symbol from an ET_REL ELF that isn't referenced within that ELF, the file can still be used in a link, you just can't reference anything that wasn't already referenced by that object file. Stripping symbols from ET_DYN/EXEC is okay, as it should have zero impact on the runtime behaviour (although debugging etc might not be as trivial). Anything that requires symbols at runtime should be using dynamic relocations and dynamic symbols, which this behaviour won't touch.

I think we need to be slightly wary of using ET_* types directly, because it will make it hard for vendors to add their own ET_* types to the list of things that should/should not be affected by some behaviour. If we do decide to go down the route of matching objcopy's behaviour here and therefore need to distinguish between relocatable and executable types, I'd suggest adding helper functions to wrap them, and call those instead of testing e_type directly.

First off, I think I agree with you on ET_* and the more I think about it the more I agree. I think given a) You're preference for not using ET_ and b) my initial and now growing preference we should not use ET_. @paulsemel please do not use ET_* checks in this way (if at all really)

Second. I think I disagree with removing defined symbols using --strip-uneeded. I interpret "uneeded" as "100% not needed in any possible link" not "uneeded to fully link this". For instance a library might define lots of functions that it doesn't call and are only meant to be used by other bits of code. Such symbols are quite common and shouldn't be removed by --strip-uneeded IMO. I'm happy for --strip-uneeded-symbol to behave the way you describe however.

Ok, so we should keep defined symbols in any way, regardless the ELF type ? That's sounds really weird for me, as there is plenty of way we want to remove them (especially in ET_EXEC), but we can go for it :)

In practice, in an ET_EXEC/DYN type, there are no regular relocations, so GNU objcopy's behaviour in this area is almost the same as --strip-all, so I don't think it matters.

[paulsemel]

Not stripping Global/Weak symbols regardless the elf type.

[jhenderson]

Since the ELF type is no longer relevant, could you remove all but one of the tests, please.

test/tools/llvm-objcopy/strip-unneeded-rel.test

44 ↗

(On Diff #147468)

Please add an undefined weak and a defined global to this test. Also a STT_FILE symbol and STT_SECTION symbol to show they are not stripped.

tools/llvm-objcopy/Object.cpp

260–261

I might be inclined to make this check into a separate shared method, to avoid risk of the two diverging.

663

++Sym->RelocRefs instead of += 1?

tools/llvm-objcopy/llvm-objcopy.cpp

392

I'd refer to the "zero symbol" as the "null symbol". Ditto the section.

[paulsemel]

Applied James' suggestions

tools/llvm-objcopy/Object.cpp

260–261

What do you think about this ?

[jhenderson]

I'm not sure if it makes much sense, but could this interact badly with group section signature symbols?

tools/llvm-objcopy/Object.cpp

260–261

I'm always a bit wary of const_cast, but I think it looks okay to me.

[paulsemel]

In D46896#1104596, @jhenderson wrote:

I'm not sure if it makes much sense, but could this interact badly with group section signature symbols?

If by interact badly you mean "having an error if this situation occurs", the answer is yes badly..
But I told myself that this problem is handle in the symbol removal function, so this is not the "problem" of this option if I can say.
Btw, we need to get rid of the error throwing and handle this case correctly.

[jakehehrlich]

In D46896#1104604, @paulsemel wrote:

In D46896#1104596, @jhenderson wrote:

I'm not sure if it makes much sense, but could this interact badly with group section signature symbols?

If by interact badly you mean "having an error if this situation occurs", the answer is yes badly..
But I told myself that this problem is handle in the symbol removal function, so this is not the "problem" of this option if I can say.
Btw, we need to get rid of the error throwing and handle this case correctly.

Shouldn't we consider a symbol needed if it is referenced by a group section? I think it should add to the reference count.

tools/llvm-objcopy/Object.cpp

199

The relocation section can do this itself by storing a non-const pointer.

tools/llvm-objcopy/Object.h

441

I don't think you need this function.

[paulsemel]

Applied Jake's suggestions

[paulsemel]

Indeed, I didn't think enough about Group symbols. You are right @jhenderson @jakehehrlich :)

[jhenderson]

Could you add a test for --strip-unneeded and group sections, please.

Thinking more about testing, what's the behaviour of --strip-unneeded + --remove-section <some relocation section> and are we happy with it?

tools/llvm-objcopy/Object.h

347

As this now includes group references, it should probably be renamed. Perhaps simply "ReferenceCount" or something similar?

468

Is this really how clang-format formats this?

[paulsemel]

In D46896#1106180, @jhenderson wrote:

Could you add a test for --strip-unneeded and group sections, please.

Thinking more about testing, what's the behaviour of --strip-unneeded + --remove-section <some relocation section> and are we happy with it?

In D46896#1106180, @jhenderson wrote:

Could you add a test for --strip-unneeded and group sections, please.

Sure.

Thinking more about testing, what's the behaviour of --strip-unneeded + --remove-section <some relocation section> and are we happy with it?

For the moment, as the section removal occurs before, the symbol will be deleted, as the relocation referencing it doesn't exist anymore. In objcopy, I believe the section removal occurs after, as the symbol is not deleted.
Anyway, if this PR (https://reviews.llvm.org/D47052) is accepted, we will match objcopy behavior (I actually prefer the current way).

[paulsemel]

Applied James' suggestions.

[paulsemel]

tools/llvm-objcopy/Object.h

468

Oups.. probably missed my clang-format.. sorry about it !

[jhenderson]

LGTM, but please wait for @jakehehrlich to approve as well.

[jakehehrlich]

One little thing and this LGTM

I think this pointed out some additional refactoring that's going to be needed though and I'm already behind on previously agreed on refactors :(

tools/llvm-objcopy/Object.h

465

Please add nullptr check

[vitalybuka]

This patch does "heap-use-after-free" http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/18486/steps/check-llvm%20asan/logs/stdio

[paulsemel]

It appears that RelocationSection is destroyed after SymbolTableSection. Thus, we are using some symbol pointers that have been freed before. I think we should directly move to shared_ptr or something similar for the symbols storage.

[jakehehrlich]

I didn't think about that. I think the key thing here then is to use shared_ptr to store all symbols and then use use_count to keep track of the references.

[alexander-shaposhnikov]

@jakehehrlich
if i'm not mistaken it could be done differently, in particular, without introducing these subtle dependencies on the order of destruction of different section types.
In fact, if i'm not mistaken it's not really necessary to introduce that logic into the destructors at all.
Would the following approach work : simply calculate the values of Symbol.UseCount after the removal of sections (it would be one loop over all the relocations and group sections) ?

[alexander-shaposhnikov]

(it seems to me that using shared ownership (shared_ptr) might not be the right solution here, the model when SymbolsTable owns the symbols seems better)

[jhenderson]

In D46896#1107439, @alexshap wrote:

@jakehehrlich
if i'm not mistaken it could be done differently, in particular, without introducing these subtle dependencies on the order of destruction of different section types.
In fact, if i'm not mistaken it's not really necessary to introduce that logic into the destructors at all.
Would the following approach work : simply calculate the values of Symbol.UseCount after the removal of sections (it would be one loop over all the relocations and group sections) ?

I tentatively agree with Alex that the SymbolTable should be the owner of a symbol, and only the SymbolTable. It just feels "right"! Everything else in ELF just refers to the symbol. I think this approach could work, but I'd suggest that it becomes another section interface, so that we just iterate once through all of the sections, and each section type either does nothing (because it doesn't refer to symbols) or updates the use count of the relevant symbols - that would allow implementing of other section types that might rely on symbols, and would also mean we don't need to track what group and relocation sections we have.

[paulsemel]

Ok, I've a working version with shared_ptr (I should have checked my emails before going on the implementation :) ). What should we do ?
If we go for the new section interface, we should probably introduce it in a separate patch, right ?
Do I still update this patch with the shared_ptr waiting for a better implementation ?
If you guys don't have time to do it, I can try if you want (maybe I'll need a bit more explanation about what you have in mind :) )

cc: @jakehehrlich @jhenderson @alexshap

[jakehehrlich]

Just a heads up I'm not convinced my shared_ptr idea is ok. I want to
consider what Alex and James are saying. My hope has always been to keep
the symbol table as the owner so if Alex has a proposal I want to read it
first.

[jakehehrlich]

Yeah I agree with Alex and James here. I think I'd prefer some sort of interface extension. Something as simple as 'markSymbols' that's done as a loop though the sections which then just sets a Boolean like you had originally. That should a) give the desired time complexity and b) not be super complicated. That loop can also only be triggered if --strip-uneeded is used which is nice.

[paulsemel]

I got a bit confused about what you wanted @jakehehrlich .
Tell me if it does fit your expectations :)

[jhenderson]

I think this makes sense to me. LGTM, with one nit, but obviously please wait for @jakehehrlich to give his opinion.

tools/llvm-objcopy/Object.cpp

188

This could probably be initialised in the class inline.

[paulsemel]

Initialized Referenced field directly in the structure.

[alexander-shaposhnikov]

tools/llvm-objcopy/Object.cpp

261

probably you don't need the "internal" cast static_cast<const SymbolTableSection *>(this), do you ?

[alexander-shaposhnikov]

tools/llvm-objcopy/Object.h

631

missing override ?

[jakehehrlich]

As always the right solution winds up 10000 times simpler than anything I considered in the interim. This looks good to me after fixing Alex's and my comments.

tools/llvm-objcopy/Object.cpp

948

This can be done externally via a loop over sections(). I'd like to keep the exposed methods as simple as possible.

[paulsemel]

tools/llvm-objcopy/Object.cpp

261

I'm sorry, but I don't see how I can do this differently. A bit of help would be appreciated.
If I do not static_cast this, I will self call, no ?

948

Alright, I'll strip this function :)

tools/llvm-objcopy/Object.h

631

Actually no, this method has the same name as the markSymbols BaseSection method, but is different (I should I named it differently, anyway, @jakehehrlich wants me to remove it, so no more issues :) )

[alexander-shaposhnikov]

tools/llvm-objcopy/Object.cpp

261

oh, right, it's fine

[paulsemel]

Move away loop over sections to HandleArgs, as suggested by @jakehehrlich

[paulsemel]

Does it look ok for you @jakehehrlich ?

[jakehehrlich]

nit: Add comment above loop but otherwise LGTM. Feel free to submit after that.

tools/llvm-objcopy/llvm-objcopy.cpp

226

nit: If you could add a comment explaining what this is doing and why it needs to be here that would be great.

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

226

@paulsemel, wait, but shouldn't it be right after Obj.removeSections(RemovePred); ?
(and in that case update the symbols table again)
For example, if one removes a group of sections (including the .group section itself),
the list of "referenced" symbols might potentially change, it's necessary to add a test for this case too, i think.

[paulsemel]

tools/llvm-objcopy/llvm-objcopy.cpp

226

The problem you are mentioning is actually a bigger one I told you about in your last review... (https://reviews.llvm.org/D47052)

Indeed, you introduced the whole behavior. This behavior is now the same as objcopy's. It seems that they are first removing symbols, and then sections, which in this case remove the group section but not the symbol. As far as I recall, I gave the example of removing a relocation and in the same time remove the referenced symbols (try it with objcopy and please read the comment I'm referring to). This is the exact same problem.

As I said in the mentioned comment, I still think we shouldn't have handled options conflicts this way, but anyway, if we want to handle all of those problems differently, I'm convinced this is not about this patch, but (again) more generally about how we handle option conflicts.

[alexander-shaposhnikov]

ok

[paulsemel]

Added comment above the markSymbols loop.
Now landing.

test/tools/llvm-objcopy/strip-unneeded.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --strip-unneeded %t %t2
+# RUN: llvm-readobj -symbols %t2 | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x0000000000000010
+    Size:            64
+  - Name:            .rel.text
+    Type:            SHT_REL
+    Link:            .symtab
+    Info:            .text
+    Relocations:
+      - Offset: 0x1000
+        Symbol: foo
+        Type:   R_X86_64_PC32
+Symbols:
+  Local:
+    - Name:     foo
+      Type:     STT_FUNC
+      Section:  .text
+      Value:    0x1000
+      Size:     8
+    - Name:     bar
+      Type:     STT_FUNC
+      Size:     8
+      Section:  .text
+      Value:    0x1008
+    - Name:     fileSymbol
+      Type:     STT_FILE
+    - Name:     sectionSymbol
+      Type:     STT_SECTION
+  Weak:
+    - Name:     baz
+      Type:     STT_FUNC
+      Size:     8
+      Section:  .text
+      Value:    0x1010
+    - Name:     foobaz
+      Type:     STT_FUNC
+  Global:
+    - Name:     foobar
+      Type:     STT_FUNC
+    - Name:     barbaz
+      Type:     STT_FUNC
+      Size:     8
+      Section:  .text
+      Value:    0x1018
+
+#CHECK: Symbols [
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name:
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: None
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Undefined
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: foo
+#CHECK-NEXT:    Value: 0x1000
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: fileSymbol
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: File
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Undefined
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: sectionSymbol
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Local
+#CHECK-NEXT:    Type: Section
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Undefined
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: barbaz
+#CHECK-NEXT:    Value: 0x1018
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Global
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: baz
+#CHECK-NEXT:    Value: 0x1010
+#CHECK-NEXT:    Size: 8
+#CHECK-NEXT:    Binding: Weak
+#CHECK-NEXT:    Type: Function
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: .text
+#CHECK-NEXT:  }
+#CHECK-NEXT:]

tools/llvm-objcopy/ObjcopyOpts.td

defm strip_symbol : Eq<"strip-symbol">,
                        HelpText<"Remove symbol <symbol>">;
 def N : JoinedOrSeparate<["-"], "N">,
         Alias<strip_symbol>;
 defm keep_symbol : Eq<"keep-symbol">,
                        MetaVarName<"symbol">,
                        HelpText<"Do not remove symbol <symbol>">;
 def K : JoinedOrSeparate<["-"], "K">,
         Alias<keep_symbol>;
+def strip_unneeded : Flag<["-", "--"], "strip-unneeded">,
+                      HelpText<"Remove all symbols not needed by relocations">;

tools/llvm-objcopy/Object.h

struct Symbol {
   SymbolShndxType ShndxType;
   uint32_t Index;
   StringRef Name;
   uint32_t NameIndex;
   uint64_t Size;
   uint8_t Type;
   uint64_t Value;
   uint8_t Visibility;
+  uint32_t RelocRefs;
 
   uint16_t getShndx() const;
 };
 
 class SymbolTableSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 
   void setStrTab(StringTableSection *StrTab) { SymbolNames = StrTab; }
   void assignIndices();
 
 protected:
   std::vector<std::unique_ptr<Symbol>> Symbols;
   StringTableSection *SymbolNames = nullptr;
 
   using SymPtr = std::unique_ptr<Symbol>;
 
 public:
   void addSymbol(StringRef Name, uint8_t Bind, uint8_t Type,
                  SectionBase *DefinedIn, uint64_t Value, uint8_t Visibility,
                  uint16_t Shndx, uint64_t Sz);
   void addSymbolNames();
   const SectionBase *getStrTab() const { return SymbolNames; }
   const Symbol *getSymbolByIndex(uint32_t Index) const;
+  Symbol *getSymbolByIndex(uint32_t Index);
   void updateSymbols(function_ref<void(Symbol &)> Callable);
 
   void removeSectionReferences(const SectionBase *Sec) override;
   void initialize(SectionTableRef SecTable) override;
   void finalize() override;
   void accept(SectionVisitor &Visitor) const override;
   void removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
 
   static bool classof(const SectionBase *S) {
     return S->Type == ELF::SHT_SYMTAB;
   }
 };
 
 struct Relocation {
-  const Symbol *RelocSymbol = nullptr;
+  Symbol *RelocSymbol = nullptr;
   uint64_t Offset;
   uint64_t Addend;
   uint32_t Type;
 };
 
 // All relocation sections denote relocations to apply to another section.
 // However, some relocation sections use a dynamic symbol table and others use
 // a regular symbol table. Because the types of the two symbol tables differ in

 
 class RelocationSection
     : public RelocSectionWithSymtabBase<SymbolTableSection> {
   MAKE_SEC_WRITER_FRIEND
 
   std::vector<Relocation> Relocations;
 
 public:
+  ~RelocationSection();
   void addRelocation(Relocation Rel) { Relocations.push_back(Rel); }
   void accept(SectionVisitor &Visitor) const override;
   void removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
 
   static bool classof(const SectionBase *S) {
     if (S->Flags & ELF::SHF_ALLOC)
       return false;
     return S->Type == ELF::SHT_REL || S->Type == ELF::SHT_RELA;
   }
 };
 
 // TODO: The way stripping and groups interact is complicated
 // and still needs to be worked on.
 
 class GroupSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
   const SymbolTableSection *SymTab = nullptr;
-  const Symbol *Sym = nullptr;
+  Symbol *Sym = nullptr;
   ELF::Elf32_Word FlagWord;
   SmallVector<SectionBase *, 3> GroupMembers;
 
 public:
   // TODO: Contents is present in several classes of the hierarchy.
   // This needs to be refactored to avoid duplication.
   ArrayRef<uint8_t> Contents;
 
   explicit GroupSection(ArrayRef<uint8_t> Data) : Contents(Data) {}
+  ~GroupSection() { --Sym->RelocRefs; }
 
   void setSymTab(const SymbolTableSection *SymTabSec) { SymTab = SymTabSec; }
-  void setSymbol(const Symbol *S) { Sym = S; }
+  void setSymbol(Symbol *S) { Sym = S; ++Sym->RelocRefs; }
   void setFlagWord(ELF::Elf32_Word W) { FlagWord = W; }
   void addMember(SectionBase *Sec) { GroupMembers.push_back(Sec); }
 
   void initialize(SectionTableRef SecTable) override{};
   void accept(SectionVisitor &) const override;
   void finalize() override;
   void removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
 
public:
   ConstRange<SectionBase> sections() const {
     return make_pointee_range(Sections);
   }
   Range<Segment> segments() { return make_pointee_range(Segments); }
   ConstRange<Segment> segments() const { return make_pointee_range(Segments); }
 
   void removeSections(std::function<bool(const SectionBase &)> ToRemove);
   void removeSymbols(function_ref<bool(const Symbol &)> ToRemove);
   template <class T, class... Ts> T &addSection(Ts &&... Args) {
     auto Sec = llvm::make_unique<T>(std::forward<Ts>(Args)...);
     auto Ptr = Sec.get();
     Sections.emplace_back(std::move(Sec));
     return *Ptr;
   }
   Segment &addSegment(ArrayRef<uint8_t> Data) {
     Segments.emplace_back(llvm::make_unique<Segment>(Data));
     return *Segments.back();
   }
 };
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_OBJECT_H

tools/llvm-objcopy/Object.cpp

if (Shndx >= SHN_LORESERVE)
       Sym.ShndxType = static_cast<SymbolShndxType>(Shndx);
     else
       Sym.ShndxType = SYMBOL_SIMPLE_INDEX;
   }
   Sym.Value = Value;
   Sym.Visibility = Visibility;
   Sym.Size = Sz;
   Sym.Index = Symbols.size();
+  Sym.RelocRefs = 0;
   Symbols.emplace_back(llvm::make_unique<Symbol>(Sym));
   Size += this->EntrySize;
 }
 
 void SymbolTableSection::removeSectionReferences(const SectionBase *Sec) {
   if (SymbolNames == Sec) {
     error("String table " + SymbolNames->Name +
           " cannot be removed because it is referenced by the symbol table " +
           this->Name);
   }
   removeSymbols([Sec](const Symbol &Sym) { return Sym.DefinedIn == Sec; });
 }
 
 void SymbolTableSection::updateSymbols(function_ref<void(Symbol &)> Callable) {
   for (auto &Sym : Symbols)
     Callable(*Sym);
   std::stable_partition(
       std::begin(Symbols), std::end(Symbols),
       [](const SymPtr &Sym) { return Sym->Binding == STB_LOCAL; });

 }
 
 const Symbol *SymbolTableSection::getSymbolByIndex(uint32_t Index) const {
   if (Symbols.size() <= Index)
     error("Invalid symbol index: " + Twine(Index));
   return Symbols[Index].get();
 }
 
+Symbol *SymbolTableSection::getSymbolByIndex(uint32_t Index) {
+  return const_cast<Symbol *>(
+      static_cast<const SymbolTableSection *>(this)->getSymbolByIndex(Index));
+}
+
 template <class ELFT>
 void ELFSectionWriter<ELFT>::visit(const SymbolTableSection &Sec) {
   uint8_t *Buf = Out.getBufferStart();
   Buf += Sec.Offset;
   typename ELFT::Sym *Sym = reinterpret_cast<typename ELFT::Sym *>(Buf);
   // Loop though symbols setting each entry of the symbol table.
   for (auto &Symbol : Sec.Symbols) {
     Sym->st_name = Symbol->NameIndex;
void ELFSectionWriter<ELFT>::visit(const RelocationSection &Sec) {
   else
     writeRel(Sec.Relocations, reinterpret_cast<Elf_Rela *>(Buf));
 }
 
 void RelocationSection::accept(SectionVisitor &Visitor) const {
   Visitor.visit(*this);
 }
 
+RelocationSection::~RelocationSection() {
+  for (auto& Rel : Relocations) {
+    --Rel.RelocSymbol->RelocRefs;
+  }
+}
+
 void RelocationSection::removeSymbols(
     function_ref<bool(const Symbol &)> ToRemove) {
   for (const Relocation &Reloc : Relocations)
     if (ToRemove(*Reloc.RelocSymbol))
       error("not stripping symbol `" + Reloc.RelocSymbol->Name +
             "' because it is named in a relocation");
 }
 

 static void getAddend(uint64_t &ToSet, const Elf_Rel_Impl<ELFT, true> &Rela) {
   ToSet = Rela.r_addend;
 }
 
 template <class T>
 void initRelocations(RelocationSection *Relocs, SymbolTableSection *SymbolTable,
                      T RelRange) {
   for (const auto &Rel : RelRange) {
     Relocation ToAdd;
     ToAdd.Offset = Rel.r_offset;
     getAddend(ToAdd.Addend, Rel);
     ToAdd.Type = Rel.getType(false);
-    ToAdd.RelocSymbol = SymbolTable->getSymbolByIndex(Rel.getSymbol(false));
+    Symbol *Sym = SymbolTable->getSymbolByIndex(Rel.getSymbol(false));
+    ++Sym->RelocRefs;
+    ToAdd.RelocSymbol = Sym;
     Relocs->addRelocation(ToAdd);
   }
 }
 
 SectionBase *SectionTableRef::getSection(uint16_t Index, Twine ErrMsg) {
   if (Index == SHN_UNDEF || Index > Sections.size())
     error(ErrMsg);
   return Sections[Index - 1].get();
 }

 void Object::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {
   if (!SymbolTable)
     return;
 
   for (const SecPtr &Sec : Sections)
     Sec->removeSymbols(ToRemove);
 }
 
 void Object::sortSections() {
   // Put all sections in offset order. Maintain the ordering as closely as
   // possible while meeting that demand however.
   auto CompareSections = [](const SecPtr &A, const SecPtr &B) {
     return A->OriginalOffset < B->OriginalOffset;
   };
   std::stable_sort(std::begin(this->Sections), std::end(this->Sections),
                    CompareSections);
 }

tools/llvm-objcopy/llvm-objcopy.cpp

struct CopyConfig {
   std::vector<StringRef> SymbolsToKeep;
   StringMap<StringRef> SymbolsToRename;
   bool StripAll = false;
   bool StripAllGNU = false;
   bool StripDebug = false;
   bool StripSections = false;
   bool StripNonAlloc = false;
   bool StripDWO = false;
+  bool StripUnneeded = false;
   bool ExtractDWO = false;
   bool LocalizeHidden = false;
   bool Weaken = false;
   bool DiscardAll = false;
 };
 
 using SectionPred = std::function<bool(const SectionBase &Sec)>;
 

 // system. The only priority is that keeps/copies overrule removes.
 void HandleArgs(const CopyConfig &Config, Object &Obj, const Reader &Reader,
                 ElfType OutputElfType) {
 
   if (!Config.SplitDWO.empty()) {
     SplitDWOToFile(Config, Reader, Config.SplitDWO, OutputElfType);
   }
 
   SectionPred RemovePred = [](const SectionBase &) { return false; };
 
   // Removes:
   if (!Config.ToRemove.empty()) {
     RemovePred = [&Config](const SectionBase &Sec) {
       return std::find(std::begin(Config.ToRemove), std::end(Config.ToRemove),
                        Sec.Name) != std::end(Config.ToRemove);
     };
   }
Obj.removeSymbols([&](const Symbol &Sym) {
           Sym.Type != STT_SECTION)
         return true;
 
       if (!Config.SymbolsToRemove.empty() &&
           is_contained(Config.SymbolsToRemove, Sym.Name)) {
         return true;
       }
 
+      // TODO: We might handle the 'null symbol' in a different way
+      // by probably handling it the same way as we handle 'null section' ?
+      if (Config.StripUnneeded && !Sym.RelocRefs && Sym.Index != 0 &&
+          (Sym.Binding == STB_LOCAL || Sym.getShndx() == SHN_UNDEF) &&
+          Sym.Type != STT_FILE && Sym.Type != STT_SECTION)
+        return true;
+
       return false;
     });
   }
 }
 
 std::unique_ptr<Reader> CreateReader(StringRef InputFilename,
                                      ElfType &OutputElfType) {
   // Right now we can only read ELF files so there's only one reader;
CopyConfig ParseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   for (auto Arg : InputArgs.filtered(OBJCOPY_add_section))
     Config.AddSection.push_back(Arg->getValue());
   Config.StripAll = InputArgs.hasArg(OBJCOPY_strip_all);
   Config.StripAllGNU = InputArgs.hasArg(OBJCOPY_strip_all_gnu);
   Config.StripDebug = InputArgs.hasArg(OBJCOPY_strip_debug);
   Config.StripDWO = InputArgs.hasArg(OBJCOPY_strip_dwo);
   Config.StripSections = InputArgs.hasArg(OBJCOPY_strip_sections);
   Config.StripNonAlloc = InputArgs.hasArg(OBJCOPY_strip_non_alloc);
+  Config.StripUnneeded = InputArgs.hasArg(OBJCOPY_strip_unneeded);
   Config.ExtractDWO = InputArgs.hasArg(OBJCOPY_extract_dwo);
   Config.LocalizeHidden = InputArgs.hasArg(OBJCOPY_localize_hidden);
   Config.Weaken = InputArgs.hasArg(OBJCOPY_weaken);
   Config.DiscardAll = InputArgs.hasArg(OBJCOPY_discard_all);
   for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbol))
     Config.SymbolsToLocalize.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_globalize_symbol))
     Config.SymbolsToGlobalize.push_back(Arg->getValue());