Page MenuHomePhabricator

[llvm-readobj][XCOFF] Add support for `--needed-libs` option.
ClosedPublic

Authored by Esme on Jul 23 2021, 3:01 AM.

Details

Summary

This patch is trying to add support for llvm-readobj --needed-libs option under XCOFF.
For XCOFF, the needed libraries can be found from the Import File ID Name Table of the Loader Section.
Currently, I am using binary inputs in the test since yaml2obj does not yet support for writing the Loader Section and the import file table.

Diff Detail

Event Timeline

Esme created this revision.Jul 23 2021, 3:01 AM
Esme requested review of this revision.Jul 23 2021, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2021, 3:01 AM
Esme updated this revision to Diff 361144.Jul 23 2021, 3:03 AM
Esme updated this revision to Diff 361145.Jul 23 2021, 3:11 AM

You'll need an XCOFF developer to review the object file code.

llvm/lib/Object/XCOFFObjectFile.cpp
830

This more closely represents the literal string you'd specify.

llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test
2

Add simple comment to the start of the test.

Also add test cases where there are no entries in the table (it is empty), and no table at all (assuming neither case is explicitly forbidden by the spec).

3

For both FileCheck invocations, I'd add --strict-whitespace --match-full-lines to ensure you are capturing the whole NeedLibraries output.

8–9

Use CHECK-NEXT.

Also, update your test inputs to have more than one entry.

12

a) The leading comma without any content looks wrong
b) If you craft your inputs differently, you could have the same output for both forms and avoid the need for two different checks.
c) Where are the spaces after the commas coming from in this output? The code doesn't seem to add them currently.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
501

Test case required.

507–518

Using a for loop seems like the right thing to do here. Also:

  1. Don't use uint32_t for a count, use size_t.
  2. Why are you omitting the LIBPATH value? It seems like you should have some method to dump this.
  3. Index from 0 or people are going to think you have an off-by-one error.
  4. No need to use comments to describe what the code clearly does.
  5. Add a space after the comma?
Esme updated this revision to Diff 362703.Jul 29 2021, 3:37 AM
Esme marked 4 inline comments as done.

Addressed comments.

llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test
2

According to the spec, no entries in the table is the same as no table at all, because a table consists only of entries.

12

a) Because there is no path name in some import ID entries. Should I not print the comma for an empty name? The output format references the output of the system objdump in AIX.

$ objdump -P loader 1.o
...
Import files:
  0: /home/esme/workspace/build/bin/../lib/powerpc-ibm-aix7.2.0.0:/home/esme/workspace/build/bin/../xlmass/latest/lib/:/home/esme/workspace/build/bin/../lib/:/usr/lib:/lib,,
  1: ,libc.a,shr.o

c) Spaces are automatically generated by ScopedPrinter when using startLine()<<.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
501

I will add cases to test wrong paths after the import file table can be customized in yaml2obj.

507–518

Thanks!

  1. What I omitted here is /home/esme/workspace/build/bin/../lib/powerpc-ibm-aix7.2.0.0:/home/esme/workspace/build/bin/../xlmass/latest/lib/:/home/esme/workspace/build/bin/../lib/:/usr/lib:/lib\0\0\0, which contains a default LIBPATH name without file name or archive member name, and I think we don't need to dump it in needed libs? Btw, ELF doesn't print paths under --needed-libs either.
  1. W.startLine() << will generate 2 spaces in the case.
void printIndent() {
  OS << Prefix;
  for (int i = 0; i < IndentLevel; ++i)
    OS << "  ";
}

Apologies, I haven't had a chance to get back to this, and I'm off work next week, so won't be able to until after that.

Esme added a comment.Aug 6 2021, 1:51 AM

Apologies, I haven't had a chance to get back to this, and I'm off work next week, so won't be able to until after that.

Never mind! Thanks for your review.

Some minor comments. Thanks for doing this.

llvm/include/llvm/Object/XCOFFObjectFile.h
110

32 bit loader section should have no l_rldoff field?

123

The padding seems not follow XCOFF doc. As I see, there should be 16 byte padding between OffsetToSymTbl and OffsetToRelEnt?

llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test
8–9

Would it better to add a header for the contents, like:

PATH            BASE            MEMBER     
,            libc.a,            shr.o
,            libunwind.a,            libunwind.so.1
,            libc++abi.a,            libc++abi.so.1

And then, maybe we can use the alignment to tell which value is for which field. so we don't need the ,?

For me, it is a little strange with , as the start or end of an import file entry.

We may generate strings like:

,    libxxx.a,
,  libunwind.a,  libunwind.so.1
Esme updated this revision to Diff 365655.Aug 10 2021, 9:42 PM

Address comments.

llvm/include/llvm/Object/XCOFFObjectFile.h
110

According to the spec, there is no l_symoff(OffsetToSymTbl) in 32-bit Loader section, but there is l_rldoff(OffsetToRelEnt).

Esme updated this revision to Diff 365682.Aug 11 2021, 12:25 AM

Remove l_rldoff from LoaderSectionHeader32.

llvm/include/llvm/Object/XCOFFObjectFile.h
110

Ok, sorry, it seems the spec has some inconsistency on LoaderSectionHeader32, and you're right that 32-bit has no l_rldoff.

I like the new output format better. That being said, I think you need to be careful of really long filenames. I don't know what's the best approach here, but one option would be to parse all the names before printing the headers, and then adjust the column widths accordingly. Up to you though.

llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test
11

If the PATH column is always empty, then there's no point in having it in the header. If it isn't, we need a test case showing that things can be printed in this field.

Esme updated this revision to Diff 366847.Aug 17 2021, 3:07 AM

Address comment.

Esme updated this revision to Diff 366870.Aug 17 2021, 5:57 AM
Esme updated this revision to Diff 366874.Aug 17 2021, 6:09 AM

I think you need testing for long names in your test case, to show the column width adjustments.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
497

The name here is somewhat generic, but the functionality is tied to the needed libraries implementation (i.e. it's to do with the base/member columns specifically). It probably makes more sense to just inline this function, since it's only caleld once.

Esme added a comment.Aug 18 2021, 1:05 AM

I think you need testing for long names in your test case, to show the column width adjustments.

I think the test case is enough? Because the default width is 13, and libunwind.so.1's width is 14.

Esme updated this revision to Diff 367147.Aug 18 2021, 1:53 AM

Remove the standalone function of getting max width.

I think you need testing for long names in your test case, to show the column width adjustments.

I think the test case is enough? Because the default width is 13, and libunwind.so.1's width is 14.

That's only the Member column. The Base column needs the same testing, as it is in fact the one that's more important. In fact, you could probably drop references to MemberWidth, thinking about it, since it has no impact on anything useful, as far as I understand it - having trailing spaces on the column is not useful. Thoughts?

Esme updated this revision to Diff 368036.Aug 22 2021, 10:43 PM

Address comment.

jhenderson added inline comments.Aug 23 2021, 12:08 AM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
529–533
  1. You don't want to justify/add trailing spaces on the last column - there's no point.
  2. You can simplify this code.
Esme updated this revision to Diff 368042.Aug 23 2021, 12:44 AM

Address comment and modify tests.

This revision is now accepted and ready to land.Aug 23 2021, 1:02 AM
This revision was landed with ongoing or failed builds.Thu, Aug 26, 12:17 AM
This revision was automatically updated to reflect the committed changes.