I just ran into https://bugs.llvm.org/show_bug.cgi?id=40818, which is preventing me landing this change as is. I could modify the llvm-objcopy compress-debug-sections.yaml file to not reference a symbol at all, but I think that loses some of the intention of that test (plus it would just hide the bug).

Address review comments.

I'll take a look at this next week, if nobody else does in the meantime.

I agree with @rupprecht on this. It gets even more involved when multiple build scripts start interacting with each other, as it may not even be clear to the developer that they are using both switches. I would be happy either with --change-start changing the value set by --set-start, or a warning or error.

LGTM.

Update comment. Rebase for new yaml2obj support for SHT_GNU_verdef sections.

According to the documentation (https://llvm.org/docs/CommandLine.html#controlling-other-formatting-options) this is illegal even for the last value:

Note that cl::Grouping options cannot have values.

• Add more comments.
• Rebase to remove FIXMEs relating to SHT_MIPS_DWARF.
• Rebase to add required fields for verneed sections.

Okay, LGTM.

I'd normally ask for a test, but as D58457 has yet to land, I'll fix that up!

Add comments.

I'm surprised at just how much slower we are than GNU objcopy still, but this is definitely a big win.

LGTM, although I don't really know anything about the ARM register stuff at all. Perhaps worth getting a second person to review before landing.

Thanks @grimar! I'm going to sit on this for a day or two and see if any other reviewers want to chime in before I go ahead.

By the way, I could see a use for a validate/no-validate option in other cases. Perhaps with the Index field it would validate that the index is either a valid section index or a reserved value.

In D58445#1404051, @grimar wrote:

I thought before about some kind of conflict we might see in situations like this.
From one side we might want to keep (and maybe improve) the validating because it is probably a useful thing in general.
But we still want to have a way to produce invalid outputs when we want, as this patch does.

Should we instead of removing the validation code add an option to yaml2obj, something like -no-validate?

Hmm... interesting thought. In this case, I think if you've explicitly used Index, you probably know what you are doing by specifying an explicit section index outside the reserved range, instead of using one of the SHN_* values or using the Section field. I could be persuaded otherwise though. On a related note, without this change, you cannot specify Index: SHN_UNDEF, because SHN_UNDEF is less than SHN_LORESERVE. It's not strictly necessary, because the default is undefined, but it's still slightly weird behaviour.

Explicitly only accept .test and .yaml tests.

Yes, you're probably right. I'll do that instead.

LGTM.

Turns out that the yaml2obj config file was also broken, and I just traded one bad thing for another. The suffixes list was being completely overwritten by a list containing only '.yaml', so only tests ending in .yaml were being run. This meant that we replaced 3 running tests with 5, when it should in fact be 8 in obj2yaml. I've fixed the yaml2obj config here too, increasing the number of tests from 27 to 29.

Update config files.

LGTM.

Oops, didn't see your later responses. Thanks!

Not quite sure what was/is going on. I ran my diff script incorrectly before, hence the "C:" bit. I don't know why this file is showing up as modified rather than added. I've uploaded a diff with the path cleaned up.

LGTM.

LGTM.

LGTM, with two small comments.

LGTM.

LGTM.

In D58173#1401364, @jakehehrlich wrote:

1. What's the use case for this?
2. What's the interaction for program headers?
   
   For relocatable files I'd expect addresses to be ignored and for program headers I'd like to put more thought into this. In general, I'd like to take a stance that we not implement flags that we don't have a use case for.

That was needed in this patch since .gnu.version_r adds strings to .dynsym.

I hope you're not adding any strings to .dynsym ;-) Also, your link in the description points to verdef sections, rather than verneed.

Before relanding this @ormris, it's probably worth getting @rafauler to confirm he's okay with the option name change, as he was the original one to add it. I've added him as a reviewer. Personally, I think the justification is good, but if it is going to cause horrendous pain, perhaps we should consider alternatives?

I guess this is okay, but I'm wondering if it's gone too far? The majority of cases you've changed here are in the driver layer, so wouldn't likely ever be librarified. What's the motivation for this round?

Don't know if you're planning on using the description as your commit message (I usually do), but there's a typo in it: abyssmal -> abysmal.

Some things that can go wrong if we allow segments to be modified such that their size or addresses change:

In D57975#1400108, @amontanez wrote:

Rebase, reduced patch contents to only include one feature addition. Rather than using a unified Value field, there's now Value and String fields. This was done because a unified Value field that could accept strings and numbers made it somewhat difficult for obj2yaml to output an appropriately formatted value. This is up for discussion, though, as I'm not sure what I've done is the best solution.

Honestly, I feel that a unified field would be better (note, I haven't thought much about how that would affect the implementation). I think that matches what we already do with other fields like Section's Info etc. It is also more intuitive, and you don't have to handle the case of both being specified. I also don't think it matters if yaml2obj -> obj2yaml produces the output identical to the input, as long as it is still valid. In this case, it could (should?) use the actual address of the referenced section, rather than trying to figure out the section name.

FWIW, I think this test belongs in the DebugInfo directory of the testsuite rather than tools/llvm-symbolizer, because it is testing functionality in the library rather than in the tool layer.

LGTM, with the comment fix.