LGTM.

In D94560#2504184, @grimar wrote:

In D94560#2502703, @inglorion wrote:

Thanks again.

I chose DAG on purpose because I think it more accurately expresses what we're really asserting.

First, we since we removed the sorting, we're not guaranteeing that these will be in any particular order. Given that, I would rather not make the test depend on the order.

Actually we give a guarantee: the order of output sections must be stable. When using -NEXT we verify that we print section sizes in the same order as they present in the file.
Using -DAG relaxes this check. Also, the current version of the test case looks like we have a sorting, though we don't.

So, personally I also think that using -NEXT is would be better here. Also wonder what @jhenderson thinks.

Maybe a silly suggestion (and one that would need more discussion on the mailing lists), but why not just change the default version to 5 more widely? How often do people deliberately deviate from the default DWARF version in practice? To me, the proposed change feels like a bit of a hack for what is likely fairly unusual usage, and is only temporary, since likely most toolchains will want to change to DWARF v5 at some point going forward.

Looks fine to me, with the caveat that I don't really know much about the functionality, so others with more knowledge should approve too.

Mostly looks fine. It might be nice if we could avoid putting so much code in the ELF.h header file, but that would require more refactoring, so we probably want to leave that to another patch.

In D94560#2500343, @grimar wrote:

In D94560#2499115, @inglorion wrote:

In D94560#2497416, @jhenderson wrote:

I wonder whether the issue could be better solved by adding something (probably output section sizes as @grimar suggests) to the verbose output?

Not sure I understand what precisely you are suggesting. Are you saying to only show this information when --verbose is passed?

I think this is what @jhenderson meant, yes.

Okay, LGTM!

LGTM.

LGTM, with nit.

Looks good.

Is it important that the .dynsym is used directly when .hash/.gnu.hash is available? At the moment, your test doesn't really show whether it is used or whether you go straight to the dynamic tags for details, I believe - to do that, you'd need to have one given bogus values for the size, and show that this is not sued. Additionally, you have no testing for your error messages, nor do you have testing that shows that the .gnu.hash is used above the .hash section.

I wonder whether the issue could be better solved by adding something (probably output section sizes as @grimar suggests) to the verbose output? That would allow us to keep the error message more concise, whilst still providing a developer the ability to answer the question of "where does the data go?"

That's a good point about the address/offset comment. I'm happy with the other idea then. @MaskRay, do you have any thoughts?

In D93678#2492777, @grimar wrote:

In D93678#2476962, @jhenderson wrote:

Option 3: Have a separate "Layout" block within the YAML which would allow you to define the layout. It might look a bit like this:

Layout:
  - FileHeader
  - Section1
  - ProgramHeaders
  - Section2
  - SectionHeaders
  - Section3

I'm not sure how this option would work alongside Offset values for sections, or what should happen if e.g. the file header entry were to be omitted.

If we really want to add a way to select where program headers are placed and also to support placing both program headers and section headers at an arbitrary offsets,
then I see the following solution:

Currently, internally, we have a list of Chunks, which consists of sections and a special Fill chunk.
If we rename the "Sections" YAML key to "Layout" (or alike), then we might be able to introduce 2 more special optional chunks: ProgramHeaders and SectionHeaders.

Then we will be able to write YAMLs like this:

--- !ELF
FileHeader:
  Class: ELFCLASS64
  Data:  ELFDATA2LSB
  Type:  ET_DYN
Layout:
  - Name: .section1
    Type: SHT_PROGBITS
    Size: 0
  - Type: SectionHeaders
     Offset: 0x1000
  - Name: .section2
     Type: SHT_PROGBITS
     Offset: 0x1200
  - Type: ProgramHeaders

I.e. it might look kind of close to what you've suggested in "Option 3", but doesn't introduce a one more separate YAML block, but allows to reuse the "Sections" block.

Perhaps, we can just have the "Sections" name unchanged for now and think about changing it (or keeping) later, when the functionality will be committed.

LGTM.

LGTM.

LGTM.

LGTM.

LGTM.

In D93362#2492101, @haowei wrote:

I rebased the code, it seems upset the Phabricator since it does not distinguish between rebase changes and actual changes.

I've not reviewed the testing yet, but my immediate thought is that there needs to be a lot more, handling all the different code paths.

The testing for this is going to be somewhat similar to various llvm-readobj examples, which use yaml2obj to generate an ELF object with the required properties. For example, llvm\test\tools\llvm-readobj\ELF\dyn-symbols-size-from-hash-table.test contains testing for the llvm-readobj handling of dynamic symbols when there are no section headers. There are actually further improvements that can be made beyond that. For example, yaml2obj has recently gained the ability to not emit the section header table, rather than needing to use llvm-strip to remove it. See llvm\test\tools\yaml2obj\ELF\section-headers.yaml for an example.

LGTM.

In D93362#2480753, @haowei wrote:

friendly ping.

Personally, I think the new behaviour you're adding makes more sense. The old behaviour could get particularly confusing, I imagine, when matching literal tabs/spaces (e.g. due to --strict-whitespace) etc if the marker moves on later than expected. LGTM, but I think it would be good to get one or two others to give their input.

Could you give a step-by-step example of how you might use the information you want to emit? That will help us ensure that any proposals satisfy the requirements (or possibly to suggest an alternative).

If this brings us closer to GNU behaviour, that's fine, but I do find it weird that a build system would be doing anything involving sudo to run llvm-objcopy/llvm-strip.

Phabricator is still saying there are unit tests failing. Are these up-to-date reports, or is Phabricator stale?

I agree with @MaskRay: does the map file solve the problem for you?

LGTM.

LGTM!

Seems to me like you should have a dedicate yaml2obj test that shows the sh_entsize can be set for group sections?

LGTM!

The original split of the two styles came in D14128, by @khemant. I can't see any explanation in that review for taking the route of using a separate style class as opposed to simply doing what you're doing now. Possibly it was just easier to retrofit it in that way. The style/dumper split would make sense, if the styles were shared between the different dumper kinds, but they aren't, and I suspect the dumping functionality for other tools may be too divergent for them to be easily reusable.

Could you explain why this patch fixes the ARM_EXIDX sh_entsize specifically? I don't see anything immediately that would only fix that section (and not other sections).

If there's a case where this is useful, I definitely support it. More generally, we may want to be able to do the same with program header table placement. I don't know if we have a use-case for that yet, but it's probably worth keeping it in mind as we design this. BeforeSecData is therefore a little bit ambiguous here. For example, between the ELF header and program header table is also "before" the section data. Maybe the key words should be AfterEhdr, AfterPhdrs, AfterSections (or possibly AtEOF for the last one). We could probably not have one of AfterEhdr or AfterPhdrs for now. Longer-term, I could imagine it might be useful to allow arbitrary offsets for the section header table, so we should consider this in our design too.

Sorry for the delay in responding. I and I suspect others who were involved on this have been off over the Christmas period.

LGTM. Please wait for @grimar.

What others said - there are six cases we should care about:

1. Section headers are stripped and the ELF has no hash/gnu_hash dynamic tag - this can be treated as an error, I think since the ELF gABI requires a hash table.
2. Section headers are stripped, but there is a DT_HASH
3. Section headers are stripped, but there is a DT_GNU_HASH
4. Section headers present but there is no .hash or .gnu.hash section - this might be treated as an error, I think since the ELF gABI requires a hash table.
5. Section headers present with .hash section
6. Section headers present with .gnu.hash section

FYI, my last day working this year is tomorrow. I won't be certain to be able to review anything now until the New Year.

LGTM, with nit.

Mostly looks fine, although there's a unit test failure, so not formally LGTMing. As I think I mentioned earlier, I'm not really familiar with the functional aspects of this change, so don't rely on me alone anyway. Tomorrow will be my last day in the office for over two weeks, so feel free to land if you think others have reviewed sufficiently after the remaining test fix.

In D89845#2455700, @clayborg wrote:

In D89845#2454273, @jhenderson wrote:

Looks fine aside from the two outstanding comments re. error messages.

I commented that these are in dump functions that were already created before this patch. They are in the process of dumping the contents to the stream, so dumping the error messages to the stream seems most important. We don't want to stop dumping the information that people asked for due to running into a single error in a file as people will probably want see all of the errors in a file.

LGTM, but I wouldn't mind @psmith or someone else with more AArch64 knowledge confirming the calculation is correct.

Given that I've gone cold on this change and need to go through it again properly, and the number of other reviews that are on my plate, in addition to higher priority internal work I need to do before the Christmas break (my last day is Friday), I doubt I'll get a chance to look at this until some time in the New Year. Sorry! If people feel it's important to land sooner than that, please don't let my lack of review block this (I can always do post-commit reviews if needed).

LGTM, with @MaskRay's suggestion. We can work on bubbling up the Error/Expected in due course to get rid of the report_fatal_error calls.

One test suggestion. Otherwise, this looks fine.

Looks good, with one possible set of suggestions.

LGTM.

Sorry, this somehow slipped off my radar, and I've run out of time to do any further significant reviewing today. I've skimmed it briefly, and the general approach looks fine. I'd like ideally to find a way to avoid needing to pass both a pointer and size around for the shndx table, even if it's just to wrap it in a new class, or change to Optional<ArrayRef<...>> or something. If you'd like me to take a more detailed look tomorrow, I can certainly do so.

Sorry, this somehow slipped off my radar, and I've run out of time to do any further significant reviewing today. I've skimmed it briefly, and the general approach looks fine. I'd like ideally to find a way to avoid needing to pass both a pointer and size around for the shndx table, even if it's just to wrap it in a new class, or change to Optional<ArrayRef<...>> or something. If you'd like me to take a more detailed look tomorrow, I can certainly do so.

Looks good to me aside from @grimar's comment.

Looks fine aside from the two outstanding comments re. error messages.

Looks like there are plenty of failing tests still?

Does this change impact reading a dynamic symbol table for an object with no section header table? In such a case, the size is derived from the hash table, so I imagine it'll work the same, but ideally it should be tested.

Obviously, this looks good to me, since this was my original suggestion, but it's probably best for @MaskRay or others to agree.

In D92052#2451933, @tstellar wrote:

@jhenderson What's your opinion on backporting this?

In D93096#2451814, @grimar wrote:

I guess it might be OK. It is probably a bit inconistent with GNU which does:

static char *
get_file_type (unsigned e_type)
{
  static char buff[32];

  switch (e_type)
    {
    case ET_NONE: return _("NONE (None)");
    case ET_REL:  return _("REL (Relocatable file)");
    case ET_EXEC: return _("EXEC (Executable file)");
    case ET_DYN:  return _("DYN (Shared object file)");
    case ET_CORE: return _("CORE (Core file)");

    default:
      if ((e_type >= ET_LOPROC) && (e_type <= ET_HIPROC))
	snprintf (buff, sizeof (buff), _("Processor Specific: (%x)"), e_type);
      else if ((e_type >= ET_LOOS) && (e_type <= ET_HIOS))
	snprintf (buff, sizeof (buff), _("OS Specific: (%x)"), e_type);
      else
	snprintf (buff, sizeof (buff), _("<unknown>: %x"), e_type);
      return buff;
    }
}

We've just run into an unexpected side-effect of this change, when this got merged into our downstream branch. We have a number of new ET_* values in the OS-specific range. In our downstream branch, we've added new entries to ElfObjectFileType so that these values are printed nicely as Type: OurCustomType. However, with this change, this has changed to Type: OS Specific: (OurCustomType) which isn't ideal. We can probably live with it, but I wonder if a small refactor might be better to allow us to have a clean downstream patch without really harming upstream code. My initial suggestion would be to change to something like this:

In D92052#2449063, @burnus wrote:

As it is a regression and affects real-world code (https://bugs.llvm.org/show_bug.cgi?id=48201),
I like to backport it to LLVM 11. – Any reasons not to do so?

@tstellar Can this still go into LLVM 11.0.1? It is a very small fix for a regression and I see that https://bugs.llvm.org/show_bug.cgi?id=release-11.0.1
still lists 18 open bugs.

Could you clarify the actual intended use-case of this option? I'm just trying to understand the situation before suggesting more things, as I realise some suggestions aren't useful for specific use-cases.

Looks good to me.

No more comments from me at this time, but as before, I'd prefer others to confirm they're happy with the concept.

Seems reasonable to me. I guess in some of the existing tests, they should be checking the addend in the relocated data, but if they don't already, we aren't making the situation worse. LGTM, with nits.

In D92052#2448057, @burnus wrote:

@jhenderson – did I answer your questions - or did I misread them? Are the other issues?

In D93021#2447909, @grimar wrote:

In D93021#2446766, @MaskRay wrote:

Is this desired? For

reportUniqueWarning(
          "SHT_DYNSYM section header and DT_SYMTAB disagree about "
          "the location of the dynamic symbol table");

it is fine. But for the others, a bunch of toString are added.

It cleans up the API, leaving the single reportUniqueWarning(const Twine &Msg) method.
This method has different enough signature from regular global reportWarning.
It should help to encourage people to use toString and provide more context for error messages.

Generally I think that lines like
this->reportUniqueWarning(toString(SymsOrErr.takeError())); are not ideal.
Because most of the time messages could be better if they were like:
`this->reportUniqueWarning("some context here: " + toString(SymsOrErr.takeError()));`

I think a context could be added for many if not all of of places touched.

The test is currently failing on the pre-merge bot :-)

You need testing to show that --preserve-dates doesn't preserve dates when the content has been modified.

The test is failing on the pre-merge bots. Please fix.