So, while I think this is an entirely reasonable assumption in most cases, it's also not one that provides any kind of workaround for the few cases where it's not universally true.

I had to revert this in rL360086. I'm following up offline to see if I can narrow down what the issue is.

I didn't follow the technical details, but I don't see anything wrong with moving forward on this patch. I think this seems like an interesting idea worth experimenting with.

Looks like support was recently added in rL359380, though without any tests, and was accidentally reverted.

Reverted in r359831 due to a dependency in reverting r359311

Reverted in r359830 due to a dependency in reverting r359311

Reverted in r359830, this regresses GNU compatibility.

(Also sorry for the delay; I was out last week and am doing a very slow job of un-burying myself from what's piled up)

Adding zbrid since she has context already. I'll take a look in a bit too. Thanks for the patch!

lgtm, thanks!

llvm-objcopy test looks fine

Thanks!

I think this is a reasonable change -- it isn't strictly matching GNU objcopy in my tests, but we can update it if we find out this edge case is important.

In D61272#1482825, @jakehehrlich wrote:

I recall @ruiu telling me that mips had a really odd endianness that wasn't accounted for by the standard ELFT types. Is that still true?

Just to see if I understand the comments so far: --special-syms is essentially already the default for llvm-nm (i.e. we print special symbols), but not for GNU nm. And so even though we print special symbols, a configure script that checks if $NM --special-syms is supported will fail when it shouldn't.

Sorry, this patch dropped off my radar. I'm not sure I'll get to it this or next week, but I'll make sure to review it eventually (unless ruiu does it sooner).

Sorry, I should have just accepted this before -- the option missing from strip is my only objection. The rest are just suggestions.

lgtm, just a couple minor comments

In D60644#1467710, @amyk wrote:

@rupprecht That is true. However, isn't it possible that a system's most up to date GLIBC version is still lower than say, GLIBC 2.23? Starting a conversation on which GLIBC versions are important can possibly be something that can be done.

In terms of the compiler_rt_logb test failures, I edited the test to return 0 to see the failures. I see about 104 lines that are outputted, so I've attached my results here.

compiler_rt_logb_results.txt13 KBDownload

• Use %b format to avoid whitespace handling
• Handle ref names

Ok, this helps me see what's going on. Thanks!

• Fix typos
• Use alternative constructor to avoid specifying OSABI_NONE all the time

In D60376#1468514, @MaskRay wrote:

@rupprecht Some people's editors are configured to replace tabs with spaces. And here, I want to test the tab takes exactly one column. I relanded this revision and it seems 'tr' works.

Thanks for adding this!

In D60067#1466236, @jhenderson wrote:

In D60067#1466153, @rupprecht wrote:

But I'm fine checking this in now as .rst for consistency. Worst case scenario, it doesn't render, and someone can commit a patch to fix it. Nothing will stop compiling :)

FWIW, there is actually a build bot that does the conversion, and it fails if the syntax is bad, so we should aim to get it right first time if possible.

In D60270#1464005, @evgeny777 wrote:

FYI, most of the other llvm-objcopy developers are still away following Euro LLVM

Ah, I see. Ok, there is no rush.

In D60067#1464211, @jhenderson wrote:

LGTM, but it would be worth getting a second pair of eyes on the documentation who is more familiar with how the markup works, to make sure it is correct.

Part of me thinks this is bad -- if the version of glibc is old, then the host toolchain should just upgrade it. glibc 2.23 was released in 2016-02-19, so I think using something older than that is on the border of being called "not a modern c++ toolchain".

(requesting changes for one small bit, otherwise lgtm)

In D60189#1459556, @jakehehrlich wrote:

I haven't bread or considered this patch. I just want to clarify when I think we should make changes of this type.

We should do things "the right way" at first and then comply more closely to GNU objcopy as real world cases come up. We should make minimal comparability attempts. Effectively think of it as "premature comparability isn't he root of all evil". We can optimize for compatibility when the case comes up.

As for working backwards from where we may have over optimized in the past and doing something more sensible now, that is kind of harder. To make such a change we need to know that we're not going to break anyone. Without evidence for that we have to reject such changes.

If Jordan can build all of Google's code with such a change then that is strong evidence that this is ok. Any other large or complicated code base would also be valid I think. These kinds of checks are very expensive however some should only request such evidence if the use case is very compelling.

I'm fine with the -w flag, but I haven't had time to look at tab alignment code -- it seems very hard to follow. Perhaps it should be a separate patch? This seems like two unrelated changes.

I'm out the rest of the week, I'll leave it to James to do the commit on your behalf

Thanks for sending this! I assigned PR39841 to you since this should fix that bug.

Just a couple small comments, but also LGTM

Also don't we already have an update sections loop somewhere in this already? I think this should go there to reduce the number of passes we make over the sections

I feel somewhat ambivalent about this change. The rationale makes sense (great patch description!), but these section flags provide GNU objcopy compatibility, so I'm not sure making it do things the "expected" way is actually helpful -- we should just provide separate flags if we want to do things the right way. (e.g. expose "sht_progbits" as a command line flag).

In D60134#1452819, @jhenderson wrote:

In D60134#1452806, @jhenderson wrote:

In D60134#1452786, @grimar wrote:

And this patch implements the correct behavior of the normal option the first time it seems.

Thanks. I took the behaviour from --no-demangle in llvm-symbolizer, so I can't exactly take all the credit!

We spoke too soon. Updating the test as @rupprecht suggested highlighted an issue where the switches cannot appear multiple times on the command-line. There's no good reason why they shouldn't (aside from tidiness), and I have encountered build systems and scripts where appending options to an existing command-line is about the only choice available, even if it means duplicate options, so we should allow multiple occurrences. I'll update the patch accordingly.

Thanks for splitting this change off.

In D60042#1452652, @jakehehrlich wrote:

I'm not sure I like renaming and I'm not sure anyone requires that behavior.

From the bug in the patch description, this is used by linux here: https://github.com/ClangBuiltLinux/linux/blob/master/drivers/firmware/efi/libstub/Makefile#L75

LGTM, but since this is a pretty big change, it'd be good to get someone else's opinion

Looks good to me, thanks! Do you need one of us to commit on your behalf?

• Fix some method/variable names

(adding a few others that may be interested)

Code looks good, but can you add a test for this? Adding something to test/tools/llvm-objcopy/ELF/keep-symbol.test should be fine

In D59958#1447533, @MaskRay wrote:

The intention makes sense. I just found this piece of code

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;hb=cc00a5d100973549bf5e4840937529633f4de1fa;f=bfd/elf.c#l3136

int
bfd_elf_get_default_section_type (flagword flags)
{
  if ((flags & (SEC_ALLOC | SEC_IS_COMMON)) != 0
      && (flags & (SEC_LOAD | SEC_HAS_CONTENTS)) == 0)
    return SHT_NOBITS;
  return SHT_PROGBITS;
}

Can the SectionFlag rules be simplified?

• Only change the type if the input is SHT_NOBITS
• Add test coverage

• Update test comments