[PATCH] D131309: [ELF] Add ability to get a symbol by name from the hash table section

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:916
+  Expected<ELFObjectFile<ELF64LE>> ElfOrErr = toBinary<ELF64LE>(Storage, R"(
+--- !ELF
+FileHeader:
----------------
jhenderson wrote:
> jhuber6 wrote:
> > jhenderson wrote:
> > > You might be able to avoid duplicating this string between some of the test cases, by having a factory function that constructs the string by having a prefix (which ends at the end of the section you want to modify) and a suffix (which starts immediately after it), and then an optional field that can be inserted in the middle. Essentially you could have lines like the following then in the different tests:
> > > ```
> > > makeYAMLWithInvalidDynsym("ShOffset: 9999");
> > > makeYAMLWithInvalidHash("ShSize: 1");
> > > ```
> > > or perhaps:
> > > ```
> > > makeYAML(DynsymPrefix, "ShOffset: 9999", DynsymSuffix);
> > > makeYAML(HashPrefix, "ShSize: 1", HashSuffix);
> > > ```
> > > Potentially, you could have that function construct the ELF and return the hash table too, to reduce the boiler plate.
> > The solution I had previously that modified the ELF in-place was more compact because I could iteratively make different field invalid using the same input. I could go back to that method, otherwise I think it's too much work to make some factory generator just for saving a few string constants in a test.
> Yes, I see your point. I do have a strong aversion to const_cast, but I don't really have much justification to it in this particular context. I see there is limited usage within LLVM unit tests of it too. I'd like a second opinion from a regular contributor in this area:
> 
> @MaskRay, do you have any thoughts?
I prefer avoiding `const_cast`, but for this case it's fine if an alternative needs too much boilerplate.

`SmallString<0> Storage;` and the `toBinary<ELF64LE>(Storage, R"(` use make it pretty clear the underlying object is actually writable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131309/new/

https://reviews.llvm.org/D131309