This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readelf][llvm-readobj] - Fix the indentation when printing dynamic tags.
ClosedPublic

Authored by grimar on Jan 16 2020, 7:01 AM.

Details

Summary

This change is similar to one made for llvm-objdump in D72838.

llvm-readelf/llvm-readobj tools do not align the "Name/Value" column properly.
This patch adds a logic to calculate the size of indentation on fly
to fix such issues.

Diff Detail

Event Timeline

grimar created this revision.Jan 16 2020, 7:01 AM
Herald added a project: Restricted Project. · View Herald Transcript

In both cases, I'm struggling to follow why specific numbers are used, e.g. where does the "-1" "-3" etc come from?

llvm/test/tools/llvm-readobj/ELF/dynamic-tags.test
585

"this" would be more natural than "that" here.

589

IDENT -> INDENT here and below.

llvm/tools/llvm-readobj/ELFDumper.cpp
5804

Don't know if it really matters, but shouldn't one of these strings be converted to a Twine?

grimar added a comment.EditedJan 17 2020, 4:44 AM

In both cases, I'm struggling to follow why specific numbers are used, e.g. where does the "-1" "-3" etc come from?

These numbers depends on a column names used.
Imagine we have two cases. In the first, column names are
"Type" and "Value" vs "FooType" and "Value" for the second case.

The number of indentation whitespaces would be different for these cases,
just because "FooType" is 3 characters longer.

In this patch, LLVM style-part uses
"Type" << std::string(MaxTagSize - 3, ' ') << "Name/Value\n";.

The indentation we want to print is MaxTagSize - 3 because it is ==
[MaxTagSize - the length of Type (4) + a single trailing whitespace (1)].

For the GNU style I've used -1 because it wraps tag names into '(`, ')', e.g:

Tag        Type                   Name/Value
0x00000001 (NEEDED)               Shared library: [D]

Hence the indentation length is increased by 2 because of 2 round brackets printed additionally.

Hence the identation length is increased by 2 because of 2 round brackets printed additionally.

Right, I figured it was something like that. Is there any chance of pulling the column header names into separate individual string literals, so that you can do things like sizeof(NameValueStr) or whatever. I'd recommend adding code comments where that is not practical.

grimar updated this revision to Diff 239267.Jan 21 2020, 3:06 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
5804

It would be
(Twine("%-") + std::to_string(MaxTagSize) + "s ").str();
I think the complexity added does not worth it?

jhenderson accepted this revision.Jan 21 2020, 3:10 AM

LGTM with a couple of nits.

llvm/tools/llvm-readobj/ELFDumper.cpp
4000

whitespaces -> spaces

4001

whitespace -> space

5802–5803

Same comments as above.

This revision is now accepted and ready to land.Jan 21 2020, 3:10 AM
This revision was automatically updated to reflect the committed changes.
grimar marked 2 inline comments as done.