In D50589#1198576, @jhenderson wrote:

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.

• Handle --globalize --keep-global correctly, and clean up comments

• Fix tests, add null symbol to symtab, make binary objection creation less fragile, and automatically assign section indices

• Rebase/fix other method names to be lower cased.

• Misc updates to tests/comments

In D50589#1196944, @jhenderson wrote:

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

• echo -n -> echo where it doesn't matter

Sorry for the slew of reviews. I have another one for implementing "llvm-strip -p" almost ready to go too (about the same size as this), hopefully I'll be able to send it next week.

Rebase

• Fold arch-specific file header tests into a single templated test
• Replace printf w/ echo -n
• Other code review comments

• Un-revert divtc3 (128-bit fp), revert divxc3 (80-bit fp)

Thanks for the review!

• Add Elf_Sym TODO

• Consistently use ELFT typename aliases in headers

• Extract out ehdr initialization into initEhdr()

• Add sparc/ppc to arch map

• Remove Ident from object

In D50343#1190161, @rupprecht wrote:

In D50343#1190050, @jakehehrlich wrote:

Ah so I think I'm against BinaryReader being templated against ELFT. That is required to implement getElfType() however. getElfType drives which writer we create. That shouldn't be how things work however. I'll go and add my related comments to this to the previous patch. Sorry if I didn't see what was going on in the other patch and lead you astray. This patch makes things a bit more clear.

Yeah, it's not my first choice either, but it did make the implementation easier and seemed somewhat consistent with the pattern elsewhere in this file.

• Pass MachineInfo into BinaryReader to push ELFType template down to only BinaryELFBuilder<>
• Move getElfType out of Reader (and also ELFReader), it can be implemented purely in the driver class. This make it clear that "Reader" is not tied to ELF types, even if that's all that's supported currently.

D50343 includes (some of) the changes here, so I'm officially abandoning this -- it makes more sense to discuss the design on something concrete.

In D50343#1190050, @jakehehrlich wrote:

Ah so I think I'm against BinaryReader being templated against ELFT. That is required to implement getElfType() however. getElfType drives which writer we create. That shouldn't be how things work however. I'll go and add my related comments to this to the previous patch. Sorry if I didn't see what was going on in the other patch and lead you astray. This patch makes things a bit more clear.

• Refactor Object based on code review comments

I think the changes here will make more sense with the implementation of BinaryReader::create(), so I'll hold off on this change for now. I'll revive it if it still makes sense to submit independently, but more likely I'll just roll this up into the next change. Sorry for the noise!

• Revert [rename binary -> blob]

• Rename binary -> blob

• Add llvm_unreachable to handle missing optional

• Move Reader dependency out of CreateWriter

Tweak flag message string

Okay, maybe here's my best, hopefully uncontroversial, suggestion, since it would be good to get this in for the LLVM 7.0 cut today(?): leave them in but mark them as "legacy" or for GNU compatibility e.g: Flags supported for GNU compatibility: alloc, load... in both error message and help text. If and when we add more flags, for a better/clearer UI, we can make that a separate sentence like Supported Flags: alloc, write, execinstr,... Additional flags supported for GNU compatibility: load, noload... etc.

That wording sounds good to me. I've updated the errors/tests.

LGTM with those changes.

• Cover full OS/proc flags in test, fix other comments

In D49870#1181835, @jhenderson wrote:

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:

I'm really worried about clearing the TLS flag, and to a lesser extent, the INFO_LINK flag. Clearing the TLS flag would almost certainly break the file, and I have no idea what the repercussions of clearing INFO_LINK might be. I'd be tempted to break with GNU compatibility here and not clear either of them. If we really don't want to do that, I'd recommend including a "tls" input option. I'm not sure what to do with INFO_LINK, because it affects other fields in the section header. Are you sure that the INFO_LINK and TLS flags are being cleared by the renaming and not by something else (e.g. implicit meaning of section names)? What happens if you rename e.g. .tdata.foo to .tdata.bar (thus maintaining the standard TLS naming scheme)?

I'm still not able to get SHF_TLS propagated, even with a real object file (not just something artificially created w/ yaml2obj):

• Preserve certain SHF_* flags and tidy up error messages

In D49870#1179887, @jhenderson wrote:

My main concern is still the interaction between these switches and existing flags. Specifically, I'm worried about what will happen if this is used on a section with lots of flags that cannot be controlled via this switch. A quick look at the spec shows that this will clear the following flags unconditionally: SHF_INFO_LINK, SHF_GROUP, SHF_TLS, SHF_COMPRESSED, and any OS/processor-specific flags. I haven't checked what GNU objcopy does, but in basically all of those, changing the flags could be bad, and have a negative impact on the ability to link the resultant object. Could you investigate what GNU does in each of these cases, and write tests to show the behaviour, please.

• Change error messages to match gnu binutils

In D49927#1178659, @ldionne wrote:

Just to make sure I understand properly: this means we will use newlib's implementation of iswcntrl_l & friends instead of our own emulation (which is an ODR violation currently going unnoticed)? And this is OK because newlib provides iswcntrl_l & friends starting at version 2.4.

Yes, that's correct.
I clarified the description: when the methods were added, the tree was configured with NEWLIB_MINOR_VERSION = 4, meaning this wasn't released until the next version, newlib 2.5.

• Clean up long test case, fix some naming

What is the behaviour of --rename-section when it touches a section with existing flags that don't conflict with the specified value? For example alloc when the section has SHF_EXECINSTR already? Does it preserve those flags?

It looks like SHF_EXECINSTR is not preserved in that case, e.g.

Refactored/addressed code review comments

Thanks for the review! I'm clearly still ramping up on this codebase :)

• Exclude 80-bit floating point and fix ppc comment

• Inline methods to fp_lib.h and fix the ppc logb call

In D49514#1172432, @efriedma wrote:

Maybe just make this a static inline function in fp_lib.h?

Thanks, done. That also makes these symbols not exported by the library, which is nice.

Looks like this caused some test failures (http://lab.llvm.org:8011/builders/clang-cmake-aarch64-quick/builds/14441/steps/ninja%20check%201), e.g.:

Ping -- Eli, do you have time to review this, or add someone else that can take a look if you're busy?

Thanks for the suggestion; removing the Object method will probably make the next change (updating flags) simpler.

Revert changes to Object; do the renaming in objcopy directly.

Fixed indentation

Addressed all comments

Fixing no trailing \n in a test