[PATCH] D136787: [XCOFF] Decode the relocation entries of loader section of xcoff for llvm-readobj
Digger Lin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 23 13:09:15 PST 2022
DiggerLin marked an inline comment as done.
DiggerLin added inline comments.
================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:253
+ support::ubig32_t LengthOfStrTbl;
+ support::big32_t OffsetToStrTbl;
+
----------------
jhenderson wrote:
> I was just looking at the XCOFF loader section header spec, and it refers to a `l_rdoff` field, i.e. the offset of the relocation entries, but this struct here doesn't seem to define an equivalent field. Is this not supposed to represent the actual loader header struct in the file? It seems to me like it is intended to, due to the way the symbol table offset is calculated. (Admittedly, the description for this field contradicts the point of having the field, so I'm not entirely sure what to make of the spec).
there is document error on (Table 5. Loader Section Header Structure) https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__vra3i31ejbau . We have discuss internally. I implement based in the AIX OS /usr/include/loader.h (When in doubt, the header files should be believed.)
error as:
1. in Table 5. Loader Section Header Structure
the field of l_rldoff (Offset to start of relocation entries) for 32 bits ,is
( -Offset: 36,
-Length: 2)
but in the loader.h, it looks do not have the field and I used the llvm-objdump --full-contents libtest.a
to decode a 32bit example object file which has loader section . it also do not has the field l_rldoff (for 32bit xcoff object, the loader section head field has 32bytes when dump the data.)
2. there is another error on offset of l_rldoff in 64bits
(-Offset: 64
-Length: 4).
the -Offset:64 should be -Offset: 48 , according to decode a loader section head of 64bits xcoff and loader.h
3. (Table 7. Loader Section Relocation Table Entry Structure.) . According to AIX OS loader.h.
There should be no field "l_value" (Address field) in "Loader Section Relocation Table Entry" both XCOFF32 and XCOFF64.
2. the Length of field "l_rtype" (Relocation type) should be 2 (not 4), and the Offset of "l_rtype" for 32bits should be "Offset:8".
the definiton for 32bit loader section header in the /usr/include/loader.h in AIX OS as
```
#ifdef __XCOFF32__
typedef struct ldhdr {
_LONG32 l_version; /* Loader section version number */
#define _CURRENT_LDR_VERSION (1)
_LONG32 l_nsyms; /* Qty of loader Symbol table entries */
_LONG32 l_nreloc; /* Qty of loader relocation table
entries */
_ULONG32 l_istlen; /* Length of loader import file id
strings */
_LONG32 l_nimpid; /* Qty of loader import file ids. */
_ULONG32 l_impoff; /* Offset to start of loader import
file id strings */
_ULONG32 l_stlen; /* Length of loader string table */
_ULONG32 l_stoff; /* Offset to start of loader string
table */
} LDHDR;
#endif /* __XCOFF32__ */
#ifdef __XCOFF64__
typedef struct _S_(ldhdr) {
_LONG32 l_version; /* Loader section version number */
#ifdef __XCOFF32__
#define _CURRENT_LDR_VERSION_64 (2)
#else
#define _CURRENT_LDR_VERSION (2)
#endif
_LONG32 l_nsyms; /* Qty of loader Symbol table entries */
_LONG32 l_nreloc; /* Qty of loader relocation table
entries */
_ULONG32 l_istlen; /* Length of loader import file id
strings */
_LONG32 l_nimpid; /* Qty of loader import file ids. */
_ULONG32 l_stlen; /* Length of loader string table */
unsigned long long l_impoff; /* Offset to start of loader import
file id strings */
unsigned long long l_stoff; /* Offset to start of loader string
table */
unsigned long long l_symoff; /* Offset to start of loader symbol
table */
unsigned long long l_rldoff; /* Offset to start of loader rlds */
} _S_(LDHDR);
#ifdef __XCOFF32__
#define LDHDRSZ_64 sizeof(LDHDR_64)
#endif
```
================
Comment at: llvm/tools/llvm-readobj/ObjDumper.h:162
+ virtual void printLoaderSection(bool PrintHeader, bool PrintSymbolTable,
+ bool PrintRelocation) {}
----------------
jhenderson wrote:
> This field should be `PrintRelocations` because there can be more than one relocation.
I think whether to use "PrintSymbolTables" instead of "PrintSymbolTable" when I implement the decoding the symbol table of loader section. I think PrintSymbolTable can explain the we want to print symbol table out. (and we only one symbol table in the loader section, we have multi symbol table entries in the symbol table. ) the same reason for the PrintRelocation. if the parameter name is "PrintRelocationEntry", we should be change to "PrintRelocationEntries"
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:306
+ uintptr_t LoaderSectionAddr) {
+
+ auto PrintLoaderSecRelocationSymIndex = [&](const auto *LoaderSecHeader,
----------------
jhenderson wrote:
> Nit
sorry about that
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:307
+
+ auto PrintLoaderSecRelocationSymIndex = [&](const auto *LoaderSecHeader,
+ const auto *LoaderSecRelEnt,
----------------
jhenderson wrote:
> This lambda is only used once. What's the motivation for it being a lambda, rather than putting the code directly inline?
agree with you.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:323-325
+ // Because there are implicit symbol index (-2,-1,0,1,2),
+ // LoaderSecRelEnt.SymbolIndex - 3 will get real symbol information from
+ // symboltable.
----------------
jhenderson wrote:
> Please make sure to reflow again after making the suggested edits.
thanks
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:347
+ makeArrayRef(RelocationTypeNameclass));
+ W.printNumber("SectionNUmber", LoaderSecRelEntPtr->SectionNum);
+ }
----------------
jhenderson wrote:
> Typo. This appears in the output. Did you just copy the output of the tool to write your test? Be careful...
sorry, yes, But I used the patch to decode the relocations of the loader section of big lib which I copied from the /usr/lib and compare with the gnu objdump and compare the output , there has the same relocations result.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:364
+
+ if (!Obj.is64Bit()) {
+ W.printEnum("Type", (uint8_t)LoaderSecRelEntPtr->Type,
----------------
jhenderson wrote:
> I'm not sure I follow why this and the `if(Obj.is64Bit())` cases need distinguishing?
the fields order of relocation entry is different between 32bit and 64bit, I just keep the same order of the object file. not keeping the same order of the object file is OK for me. If you strong suggest me to not keep the order of object file. I can change it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136787/new/
https://reviews.llvm.org/D136787
More information about the llvm-commits
mailing list