[PATCH] D105522: [llvm-readobj][XCOFF] Fix the error dumping for the first item of StringTable.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/string-table.yaml:55
+Symbols:
+  - Name: asymname
+  - Name: longname1
----------------
You don't need the too-short-to-be-in-the-table name, because that's testing yaml2obj's emission of the string table, not llvm-readobj's dumping of it.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/string-table.yaml:30
+
+## There is an empty string table with just the length field.
+## TODO: Print the string table size to check the length field.
----------------
Esme wrote:
> jhenderson wrote:
> > Why does 32-bit XCOFF generate an empty string table if it isn't used? Is that a bug in the emitter, or a spec detail?
> Hmm...I think it's neither a spec detail, nor a bug? That's just how we implement in yaml2obj. We write the string table if the symbol table isn't empty. 
> ```
> if (!Obj.Symbols.empty()) {
>     if (!writeSymbols())
>       return false;
>     // Write the string table.
>     Strings.write(W.OS);
> }
> ```
> Due to the fact that only long symbol name can be written into the string table for 32-bit XCOFF, I write the test of non-empty symbol table without long symbol names.
> As for 64-bit XCOFF, the symbol name is a required field and all symbol names will be written into the string table, so I can't write such test under 64-bit.
Okay, I'd call that a yaml2obj bug (yaml2obj shouldn't be creating things that aren't needed - doing so wastes time and disk space). That being said, it would be a useful feature if you could customise the presence and/or contents of the string table directly in yaml2obj. Something like the following:

```
## Emit a string table with specific contents. Custom contents overwrite implicitly generated contents.
--- !XCOFF
FileHeader:
  ...
StringTable:
  - Length: 10 # Force string table length field to be set to this value.
  - Strings: ["a", "b", "cd"] # Force contetns to this.
Symbols: # Optional.
  # Names use values as-if string table were implicitly generated (might be invalid).

## Don't emit a string table.
--- !XCOFF
FileHeader:
  ...
# No StringTable or Symbols element.

## Emit a string table with implicit contents, if and only if required (i.e. 32-bit long name or any 64-bit).
--- !XCOFF
FileHeader:
  ...
Symbols:
  ...

## Suppress emission of string table.
--- !XCOFF
FileHeader:
  ...
StringTable:
  Emit: false # Or perhaps Suppress: true
Symbols:
  ... # Names generated with appropriate offsets as if table emission was not suppressed.
```

This is similar in concept to how ELF yaml2obj allows changing the contents and even presence of the .strtab section (which contains symbol names), and allows for testing of more code paths (e.g. checking that a symbol name offset is valid, printing of a string table with an invalid length field etc).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105522