[PATCH] D73045: [obj2yaml] - Better indentations in the ELF output.

[George Rimar via Phabricator via llvm-commits]

grimar marked 2 inline comments as done.
grimar added a comment.

In D73045#1829853 <https://reviews.llvm.org/D73045#1829853>, @MaskRay wrote:

> I agree that a preflight pass is unavoidable if we want to improve indentation.


There was one more way I though about.
For each type handler, like:

  void MappingTraits<ELFYAML::Object>::mapping(IO &IO, ELFYAML::Object &Object) {
  ...
    IO.mapTag("!ELF", true);
    IO.mapRequired("FileHeader", Object.Header);
    IO.mapOptional("ProgramHeaders", Object.ProgramHeaders);
    IO.mapOptional("Sections", Object.Chunks);
    IO.mapOptional("Symbols", Object.Symbols);
    IO.mapOptional("DynamicSymbols", Object.DynamicSymbols);
  ....
  }

We could calculate the indentation in its code, like:
(pseudocode)

  void MappingTraits<ELFYAML::Object>::mapping(IO &IO, ELFYAML::Object &Object) {
    int MaxKey = sizeof("FileHeader") or sizeof("ProgramHeaders") if exist and not empty or ...;
    IO.setMaxPadding(MaxKey);
  ...
    IO.mapTag("!ELF", true);
    IO.mapRequired("FileHeader", Object.Header);

It could give a nice results, it is simple and does not require pre-mapping, but
it is not a generic approach, that is why I supposed it is not an option.



================
Comment at: llvm/test/tools/obj2yaml/elf-output-indentation.yaml:19
+# CHECK-NEXT:    Type:         SHT_PROGBITS
+# CHECK-NEXT:    AddressAlign: 0x0000000000000001
+# CHECK-NEXT:  - Name:        .rela.text
----------------
jhenderson wrote:
> grimar wrote:
> > Here the additional indentation appears because of `AddressAlign`, which is
> > the largest key among others. The same situation is shown below.
> > I did not investigate how much hard to to improve such places too yet.
> I definitely find it weird that .text is not indented to the same extend as .data.
Yes, though it just follows the simple rule: "find the largest key size and align others".
I am not sure what can be a good generic heuristic without knowing the context.


================
Comment at: llvm/test/tools/obj2yaml/elf-output-indentation.yaml:20-24
+# CHECK-NEXT:  - Name:        .rela.text
+# CHECK-NEXT:    Type:        SHT_RELA
+# CHECK-NEXT:    Link:        .symtab
+# CHECK-NEXT:    EntSize:     0x0000000000000018
+# CHECK-NEXT:    Info:        .text
----------------
jhenderson wrote:
> I'm guessing this block is indented because of Relocations? This doesn't seem great, given that the Reloctions line doesn't have a direct value itself.
Yeah, but the problem here is that `preflightKey` which is used to scan keys:

```
 bool preflightKey(const char *Key, bool Required, bool SameAsDefault, bool &,
                    void *&) override {
    if (Required || !SameAsDefault)
      MaxKeyLen = std::max(MaxKeyLen, strlen(Key));
    return false;
  }
```

does not have any information about values (how to know that `Relocations` is an empty list?)
Accessing to the values never happens,
because of `return false`, which prevents any more logic from execution.
I.e. The code only scans over keys on the current level and does not allow going deeper.
If it did than much more logic would be needed to be implemented for this "pre-mapping" pass.

The solutions I see are: either adopting the code in `yaml::Output` to teach it about possibility of "pre-mapping"
(the idea of this patch was to avoid touching `Output` code, I've only fixed a minor misbehaviors in `processKeyWithDefault`)
and/or implementing a smarter (and larger) `PreMappingOutput` class.




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

https://reviews.llvm.org/D73045