[PATCH] D141848: [Test] Fix YAML mapping keys duplication. NFC.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D141848#4096252 <https://reviews.llvm.org/D141848#4096252>, @asi-sc wrote:

> I tried the suggested change and it seems we faced with one more issue: `llvm-objcopy: error: Symbol table has link index of 4 which is not a string table`

Ah, the old "llvm-objcopy treats SHF_ALLOC + SHT_STRTAB as dynamic string table internally, and won't let non-dynamic symbol tables use it" issue. We'll have to try something else.

I did some playing around and came across two separate issues in the process of trying to get a working test (see https://github.com/llvm/llvm-project/issues/60448 and https://github.com/llvm/llvm-project/issues/60451 if you're interested). In the end, I came up with the following changes to the YAML:

1. Rename .symtab.dyn to .symtab. This ensures we don't run into the two SHT_SYMTAB issue described in the first of those tickets.
2. Make sure it has SHT_SYMTAB type and SHF_ALLOC flags.
3. Don't add the SHT_STRTAB section I suggested before (it's not needed).
4. Add `--allow-broken-links` to the first llvm-objcopy and llvm-strip commands that do the stripping. This will allow the implicitly-generated .strtab section to be removed, whilst still being referenced by the .symtab section.
5. Update the CHECKs to look for ".symtab" rather than ".symtab.dyn".

Hopefully that should fix the issue. The --allow-broken-links option will result in a somewhat-broken ELF object, but it doesn't matter for the test case's purpose.

> There is even one more test that suffers from it - https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test#L214 . So, I can repeat the temporary fix from this test and use SHT_STRTAB type for .symtab.dyn section. But maybe you could have a look once again and suggest better option?

There's no need to do that, as we already have a SHT_STRTAB + SHF_ALLOC section in the test case. In fact, I'd argue that the "temporary fix" in that test is simply hiding an issue, and is not providing any useful test coverage. The real fix would be to change llvm-objcopy to not treat SHT_STRTAB + SHF_ALLOC different to regular SHT_STRTAB sections somehow. However, the problem is quite complex (we need to be careful about modifying SHF_ALLOC section sizes, so lots of operations that normally work on a SHT_STRTAB section won't work on one with the SHF_ALLOC flag), so it's non-trivial to fix without reworking how llvm-objcopy reads in the ELF object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141848