[PATCH] D65240: [XCOFF][AIX] Generate symbol table entries with llvm-readobj
Hubert Tong via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 20 16:47:44 PDT 2019
hubert.reinterpretcast added inline comments.
================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:284
+class XCOFFSymbolRef {
+ DataRefImpl SymEntDataRef;
+ const XCOFFObjectFile *const OwningObjectPtr;
----------------
I meant this line (re: `const`) as well.
================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:294
+ uint8_t getNumberOfAuxEntries() const;
+ const XCOFFCSectAuxEnt32 *getXCOFFCSectAuxEnt32() const;
+ uint16_t getType() const;
----------------
Might as well use `Csect` here while we are at this for this patch (see D65159).
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:129
+ uint32_t Offset = CFileEntPtr->NameInStrTbl.Offset;
+ if (Offset < 4)
+ return StringRef(nullptr, 0);
----------------
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.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:467
+uint32_t XCOFFObjectFile::getSymbolIndex(uintptr_t SymbolEntPtr) const {
+#ifndef NDEBUG
+ if (SymbolEntPtr < reinterpret_cast<uintptr_t>(SymbolTblPtr))
----------------
`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).
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:637
+
+const XCOFFCSectAuxEnt32 *XCOFFSymbolRef::getXCOFFCSectAuxEnt32() const {
+ assert(!OwningObjectPtr->is64Bit() &&
----------------
Global replace `CSect` with `Csect` in code (note: not the same for English).
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:641
+ assert(hasCSectAuxEnt() && "No CSect Auxiliary Entry is found.");
+ uintptr_t AuxAddr = getWithOffset(
+ SymEntDataRef.p, SYM_TAB_ENTRY_SIZE * getNumberOfAuxEntries());
----------------
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.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:643
+ SymEntDataRef.p, SYM_TAB_ENTRY_SIZE * getNumberOfAuxEntries());
+ return reinterpret_cast<const XCOFFCSectAuxEnt32 *>(AuxAddr);
+}
----------------
Might make sense to call the error checking function here.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:655
+bool XCOFFSymbolRef::hasCSectAuxEnt() const {
+ XCOFF::StorageClass Sc = getStorageClass();
+ return (Sc == XCOFF::C_EXT || Sc == XCOFF::C_WEAKEXT ||
----------------
s/Sc/SC;
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:671
+ const XCOFFCSectAuxEnt32 *CSectAuxEnt = getXCOFFCSectAuxEnt32();
+ // The symbol is label definition.
+ if ((CSectAuxEnt->SymbolAlignmentAndType & SYM_TYPE_MASK) != XCOFF::XTY_LD)
----------------
Newline before the comment. Clarify the comment as saying that we are expecting function definitions to be label definitions.
================
Comment at: llvm/test/tools/llvm-readobj/xcoff-symbols.test:1
+# This file tests the ability of llvm-readobj to generate symbol table for
+# 32-bit XCOFF object file.
----------------
s/generate/display the/;
s/for/for a/;
================
Comment at: llvm/test/tools/llvm-readobj/xcoff-symbols.test:16
+SYMBOL32-NEXT: Section: N_DEBUG
+SYMBOL32-NEXT: Source Language ID: 0
+SYMBOL32-NEXT: CPU Version ID: 3
----------------
```
TB_C = 0,
TB_CPLUSPLUS = 9,
```
================
Comment at: llvm/test/tools/llvm-readobj/xcoff-symbols.test:17
+SYMBOL32-NEXT: Source Language ID: 0
+SYMBOL32-NEXT: CPU Version ID: 3
+SYMBOL32-NEXT: StorageClass: C_FILE (0x67)
----------------
```
TCPU_PPC64 = 2,
TCPU_COM = 3,
TCPU_970 = 19,
```
================
Comment at: llvm/test/tools/llvm-readobj/xcoff-symbols.test:109
+SYMBOL32-NEXT: NumberOfAuxEntries: 1
+SYMBOL32-NEXT: CSect Auxiliary Entry {
+SYMBOL32-NEXT: Index: 13
----------------
Global replace "CSect" with "CSECT" in English.
================
Comment at: llvm/test/tools/llvm-readobj/xcoff-symbols.test:426
+#
+# > cat aix_xcoff_xlc_test8.c
+#
----------------
The filename is `test8.c`.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:28
+ SymbolAlignmentMask = 0xF8,
+ SymbolTypeBits = 3
+ };
----------------
Given the way it is used, this should be something like `SymbolAlignmentBitOffset`.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:182
+ const XCOFFSectAuxEntForStat *AuxEntPtr) {
+ DictScope SymDs(W, "Sect Auxiliary Entry For Stat");
+ W.printNumber("Index",
----------------
Assert XCOFF32. This entry type does not exist for XCOFF64.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65240/new/
https://reviews.llvm.org/D65240
More information about the llvm-commits
mailing list