Initial thoughts from me are the code looks good, but this needs more documentation overall. The approach is clever, but not immediately intuitive so I feel there should be in-code comments to explain it at a high level. Sort of like your descriptions in D55864, but in-code so they are visible to everyone reading it.

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.

Oof, my email filters somehow didn't send the discussion here to my inbox. Sorry! I'm fine with this landing as-is, I can just rebase on top of it. I think the only significant deviance from the RFC is future improvements require that DynamicEntry::Val is a StringRef instead of llvm::yaml::Hex64, but I can address that later.

See D57975 for the new patch introducing support for writing .dynamic entries. It doesn't include any changes to obj2yaml, or seek to update existing tests. I'm marking this patch as "planned changes" to reduce confusion as to which should be reviewed.

This change as-is does a lot, which was what sparked the desire for a RFC. My plan was to trim down this patch once the RFC reached a general consensus so we could have the basic functionality of tag/value pairs and build on that. Further additions (like automatically adding required entries) would be addressed in subsequent changes. If you have anything you'd like to add to the RFC, feel free. I'm fine with you working to land D57691 first if that is high-priority. Otherwise, I can reduce this patch to reflect the general functionality of yours while using some nuances that will prepare for the future features.

This has been redesigned as part of the ELF TextAPI and the llvm-elfabi tool. Those interested should follow the progress of elfabi.

Fixed a comment that passed 80 character width limit.

Removing from review queues until I look into write commands requested in D55839

Added dummy DT_SYMTAB entries to tests so all tests pass again.

Changed populateSymbol() to createELFSymbol().

Cleaned up significantly, added tests, moved from WIP status to ready for review. This patch is very large. I'm open to splitting it up after a brief discussion on how people would prefer I do that.

Updated tests.

Rebase, split out bigger functions to use helper functions, add a simple test.

Rebase on license changes, fix behavior for edge case when a .gnu.hash section has no buckets.

Comments addressed.

Addressed most of your comments, but I've left the ELF/SYSV hash table in since it amounts to 12 additional lines for now. I can pull it out later if that's really needed. I feel it's relevant and small enough to leave in.

This should address most of the comments. All functions except for the single writeBinaryStub() have become static functions, and the class has been removed. I'll be updating D55864 to match these changes as soon as possible.

Now uses DT_HASH and DT_GNU_HASH to determine the size of .dynsym. A number of tests still need to be updated as this change requires that a DT_SYMTAB entry exists in the .dynamic section (which many tests are missing).

There were quite a few tests that required the .dynamic section in D55629. D55852 tests also have a number of .dynamic entries as well. Without the ability to write .dynamic entries in yaml2obj, these will continue to contain a good portion of hexidecimal section contents with annotations. This addition to yaml2obj is going to be rather low-priority for me, but I feel it's the best way to make llvm-elfabi tests more legible. It also will make creation of new tests significantly easier.

Rebase, update error message for DT_NEEDED entries that are outside the string table, and add another test.

using reinterpret_cast now. Since it looks like it might take a bit of time to land support for dynamic entries in yaml2obj, do you feel it would be fine to land this patch now and update the tests later?

Quick changes to address some of Rui's comments, and introduced support for the older raw "Content" tag so tools/llvm-objdump/private-headers-no-dynamic-segment.test doesn't fail anymore.

The goal of this change is to introduce a new type of YAML section that allows the population of .dynamic entries by providing a list of tag and value pairs. These entries are interpreted (and potentially validated) before being written to the .dynamic section.

Rebase again.

Small change from (const char *)*DynStrPtr to (const char *)DynStrPtr.get() for clarity.

Replaced addrAsOffset() with ELFFile<ELFT>::toMappedAddr(), updated tests.

I changed a lot and included the majority of what was originally going to be in a second follow-up patch.

I was thinking the same while writing this. The only argument I have is that this allows obj2yaml to print values in decimal and pointers in hexadecimal. That's a rather weak argument though. I'm totally fine with simplifying it down to just using d_val.

Cleaned up tests and made them significantly more consistent.

Closed in rL350545

The offset of .dynstr is now calculated using the DT_STRTAB address along with the program headers.

Rebase on D55629 change that addresses strings that overrun string table bounds.

Addressed feedback in D55852: strings that overrun the bounds of a string table (due to absent null terminator or incorrect string table size) are reported as errors in terminatedSubstr().

LGTM.

Rebase and use terminatedSubstr() to fetch DT_NEEDED strings from .dynstr string table.

• Mandatory DynamicEntries members are now uint64_t instead of Optional<uint64_t>, now include more checks for correctness.
  • Bound checking when creating SoName string from .dynstr table.

Rebase onto previous patch resolves most of the comments for this patch.

Small change to make functions static.

• There is no longer one function to populate each ELFStub member.
  • The .dynamic table is only scanned once.

• Rebase on changes to parent patch.
• Fix test formatting.
• Input format flags are now lowercase only.

• Fixed test formatting.

Quick bugfix in ErrorCollector::makeError() and change to make the function more concise.

• Populating ELFStub.Arch is now inline.
  • ErrorCollector.log() now prefixes each error with "error: " in red.
  • ErrorCollector.makeError() now uses joinErrors() in lieu of a specialized Error type (marked as TODO) that gives more control over how to handle each error.
  • addError() now takes a r-value reference Error.

Hopefully this clarifies the direction on some things brought up. I didn't explicitly mention that everything outside llvm-elfabi.cpp is designed as if it were part of a library. Let me know if this resolves some concerns you had or brings up new ones. I really appreciate your insight, push back again on anything you still feels needs to be changed and I'll look into it again.

Forgot to include a few changes for a couple files.

• Addressed several areas of feedback.

Small code formatting fix.

Small change to read-tbe-as-elf.test to make it easier to debug test failure.

Small change to binary-read-arch.test to make it easier to debug test failure.

• ErrorCollector now clears errors when makeError() is called.

Added:

• ErrorCollector for collectively handling multiple related errors.

• Classes removed, functions are all standalone with no state.
  • Now using Expected<std::unique_ptr<ELFStub>> for everything that would previously return a std::unique_ptr<ELFStub>.
  • Added tests to check for proper failure output.

No further comments, LGTM

I can open a new patch that does exactly this since this patch is marked as abandoned. I haven't checked to see if phabricator supports re-opening an abandoned change.