Page MenuHomePhabricator

[XCOFF][AIX] Generate symbol table entries with llvm-readobj
ClosedPublic

Authored by jasonliu on Jul 24 2019, 12:56 PM.

Details

Summary

This patch implements main entry and auxiliary entries of symbol table generation for llvm-readobj on AIX.
The source code of aix_xcoff_xlc_test8.o (compile with xlc) is:

-bash-4.2$ cat test8.c
extern int i;
extern int TestforXcoff;
extern int fun(int i);
static int static_i;
char* p="abcd";
int fun1(int j) {
  static_i++;
  j++;
  j=j+*p;
  return j;
}
int main() {
  i++;
  fun(i);
  return fun1(i);
}

Patch provided by DiggerLin. I'm posting this on his behalf as he is currently on vacation.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
llvm/lib/Object/XCOFFObjectFile.cpp
468

Top-level const on parameter types are known to cause grief with certain build compilers.

469

Type error. The answer fits into 32-bits after being divided by 18, but not necessarily before that.

469

I suggest moving this to after the error checking.

479

>=. The calculation on the RHS points "one-past-the-end".

This revision now requires changes to proceed.Aug 17 2019, 9:37 PM
llvm/include/llvm/Object/XCOFFObjectFile.h
293

Make both fields const if we don't need to modify them.

297

Parameter variable name does not match the prevalent style.

302

There's no reason to add XCoff to the set of ways we capitalize XCOFF. We already use XCOFF even when we capitalize the next word in names.

305

Newline before this to split the derived-property functions from the field accessors.

jhenderson added inline comments.Aug 20 2019, 2:30 AM
llvm/test/tools/llvm-readobj/xcoff-symbols.test
2

A few test comments from me:

Add a brief comment to the top of the file explaining the purpose of the test.

You don't need to prepend the RUN: lines with a '#' character.

Add a single blank line between the end of the RUN: lines and the start of the CHECK patterns.

I take it yaml2obj doesn't have XCOFF support? Assuming that's the case, is it possible to use something like llvm-mc to build this at test time rather than use the pre-canned binary? In general we've been trying to avoid adding pre-canned binaries to the llvm-readobj tests, as they are opaque and hard to maintain.

sfertile added inline comments.Aug 20 2019, 7:30 AM
llvm/lib/Object/XCOFFObjectFile.cpp
480

getWithOffset?

llvm/test/tools/llvm-readobj/xcoff-symbols.test
2

I take it yaml2obj doesn't have XCOFF support? Assuming that's the case, is it possible to use something like llvm-mc to build this at test time rather than use the pre-canned binary? In general we've been trying to avoid adding pre-canned binaries to the llvm-readobj tests, as they are opaque and hard to maintain.

We are using the pre-canned binaries for testing since we need the tools support first, to be able to write lit tests as we add backend XCOFF functionality. Once we have enough functionality in the XCOFFObjectWriter we can remove the pre-canned binaries and generate them with llvm-mc as you suggest.

jasonliu marked 19 inline comments as done.Aug 20 2019, 8:23 AM
jasonliu added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
122

Changed it to StabSectNum.

llvm/test/tools/llvm-readobj/xcoff-symbols.test
2

Thanks for the comments. Addressed them in the new revision.

Yes, we haven't implement XCOFF support for yaml2obj yet. And llvm-mc do not yet have enough functionality to generate all the symbol entries kind for our testing, although we are working towards that goal. And we are actually going to use llvm-readobj to test the correctness of our llvm-mc obj gen.
So there is a chicken-and-egg problem here for us. And what we are choosing to do is to use a pre-canned binary that we know is correct (generated from xlc) to test the correctness of the tooling (llvm-readobj). Then later use this tooling to test the object file generated by llc/llvm-mc.

22

Removed this for printing XCOFF32.

jasonliu updated this revision to Diff 216160.Aug 20 2019, 8:30 AM
jasonliu marked 2 inline comments as done.
jasonliu marked 2 inline comments as done.

I addressed all the comments I see.
Thanks for the reviewing.

jhenderson added inline comments.Aug 20 2019, 9:00 AM
llvm/test/tools/llvm-readobj/xcoff-symbols.test
2

Fair enough. Another way to resolve this, which was used for minidump support, was implement support in yaml2obj for XCOFF, and test using gtest unit-testing. yaml2obj can then be used to generate objects for llvm-readobj testing, which can in turn verify llvm-mc and other tools.

llvm/include/llvm/Object/XCOFFObjectFile.h
292

I meant this line (re: const) as well.

302

Might as well use Csect here while we are at this for this patch (see D65159).

llvm/lib/Object/XCOFFObjectFile.cpp
136

Please common up some of the code between this and getSymbolName. We might be able to go with one template function implementation for both of these interfaces to use.

469

getSymbolIndex is a strange place for the error checking to occur. We already got a SymbolEntPtr, and we might have used it already. We should move the error checking out to a separate function and call it where appropriate (perhaps at each place where we retrieve a handle to a symbol table entry, or perhaps only when jumping by an index).

709

Global replace CSect with Csect in code (note: not the same for English).

713

To explain the code, add a comment to indicate that the csect auxiliary entry is, for symbols that have them in XCOFF32, the last auxiliary entry.

715

Might make sense to call the error checking function here.

727

s/Sc/SC;

743

Newline before the comment. Clarify the comment as saying that we are expecting function definitions to be label definitions.

llvm/test/tools/llvm-readobj/xcoff-symbols.test
2

s/generate/display the/;
s/for/for a/;

17
TB_C = 0,
TB_CPLUSPLUS = 9,
18
TCPU_PPC64 = 2,
TCPU_COM = 3,
TCPU_970 = 19,
110

Global replace "CSect" with "CSECT" in English.

427

The filename is test8.c.

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

Given the way it is used, this should be something like SymbolAlignmentBitOffset.

149

Assert XCOFF32. This entry type does not exist for XCOFF64.

llvm/lib/Object/XCOFFObjectFile.cpp
22

This is unused, and this should be something like RELOC_LNNO_OVERFLOW if we were to have it in this patch.

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

Use static_cast.

139

If the previous line did not need the cast to do the bitwise-and, then neither does this one. Also, remove the redundant parentheses.

141

Use static_cast.

153

Overflow handling is not here, and there is no TODO.

199

Add a period to the end of the sentence.
Replace llvm_unreachable(x) with assert(false && x) here. I guess the assert makes sense to have (in place of just a TODO), so that developers can clue in when they are running this utility on an object with the unimplemented kinds.

hubert.reinterpretcast requested changes to this revision.Aug 21 2019, 4:13 PM
hubert.reinterpretcast added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
207

s/unimplmented/unimplemented/;

237

Use static_cast.

246

s/entires/entries/;
Replace the period with an ellipsis. What is here is not a sentence.

247

Yes, the name should be ".file", but I don't think there is sufficient rationale to skip processing the auxiliary entries if the name isn't.

257

s/is Function/is for a function/;

261

Can we have some rationale here for not printing the first auxiliary entry if it is not also the last?
Also, please fix the spacing, capitalization, etc.

264

Remove the " are print out" part. It is clear that it is being printed out.

266

SYM_TAB_ENTRY_SIZE might make more sense (since we are not really talking about an XCOFFSymbolEntry object).

266

Use reinterpret_cast.

277

This is unsafe. There is no statement I could find indicating that such an auxiliary entry is required, and xlc will generate such symbol table entries with no auxiliary entry.

[16]    m   0x00000094     .data     1  unamex                    _$STATIC
[17]    a4  0x00000004       0    0     SD       RW    0    0
[18]    m   0x00000094     .data     0  static                    x
288

See comments above.

290

See comments above.

294

Add a newline between the functions.

This revision now requires changes to proceed.Aug 21 2019, 4:13 PM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
272

printCsectAuxEnt32(XCOFFSymRef.getXCOFFCsectAuxEnt32()).

jasonliu updated this revision to Diff 216666.Aug 22 2019, 10:47 AM
jasonliu marked 38 inline comments as done.
jasonliu added a subscriber: DiggerLin.
jasonliu added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
22

Yes, removed it from the patch.

@DiggerLin
Please note this change might be needed for the upcoming relocation patch.

469

Hopefully, I covered all the cases that we needed to call the checker function.

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

We have

if (NumberOfAuxEntries == 0)
    return;

before this switch statement. If there is no aux entry, we would exit already before getting here.

jasonliu marked an inline comment as done.Aug 22 2019, 11:31 AM
jasonliu added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
114

Will add a new line before this function.

jasonliu marked an inline comment as done.Aug 22 2019, 11:47 AM
jasonliu added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
153

Discussed offline, the overflow handling is for section auxiliary header, not for symbol auxiliary entry.

Looking through this latest version still (this is a largish patch).

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

Ah, yes. As a further consideration, I think we should do something about having more than one auxiliary entry here.

Looks like the patch is getting close.

llvm/include/llvm/BinaryFormat/XCOFF.h
164

Use "64-bit".

llvm/include/llvm/Object/XCOFFObjectFile.h
175

This function is sufficiently generic that we can just say that it returns string table entries (and name it appropriately).

llvm/lib/Object/XCOFFObjectFile.cpp
92

Ref.p is a uintptr_t, so this can be moved to before the viewAs.

121

Use "end-of-symbol-table".

128–131

Remove "or .debug section" as this function does not cover this case (at least not yet).

141

This is more generically "Bad offset for string table entry".

162

generateStringRef got modified in an NFC patch I believe. The uses in this patch make it more obvious that the one-argument version of that function should not be named so generically. Perhaps generateXCOFFFixedNameStringRef?

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

I've experimented with XLC, and confirmed that it does not do the overflow handling for these fields. It will populate these with the low bits of the logical value.

163

I think we should treat this as code.

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

I think we can have a comment here that, unlike the corresponding fields in the section header, these fields do not handle values greater than 65535.

jasonliu updated this revision to Diff 217177.Aug 26 2019, 9:04 AM
jasonliu marked 13 inline comments as done.

Addressed all the new comments.

llvm/lib/Object/XCOFFObjectFile.cpp
162

Agree. The name should give more information on what it is trying to do.

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

Could C_STAT symbol have more than 1 aux entry? I added an assertion for that.

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

Can we make that a report_fatal_error? It is an error in the user-provided input, so we should report the error.

jasonliu marked an inline comment as done.Aug 26 2019, 10:27 AM
jasonliu added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
277

Right. I will make it a report_fatal_error in the commit or next revision.

LGTM with minor comments.

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

Newline before the comment.

336

s/could/should/;

This revision is now accepted and ready to land.Aug 26 2019, 1:33 PM
This revision was automatically updated to reflect the committed changes.