[PATCH] D136787: [XCOFF] Decode the relocation entries of loader section of xcoff for llvm-readobj

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:253
+  support::ubig32_t LengthOfStrTbl;
+  support::big32_t OffsetToStrTbl;
+
----------------
DiggerLin wrote:
> 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
> 
> ```
> 
Thanks for the explanation, it makes sense. In the future though, I'd put such lengthy comments out-of-line, as they make it harder to follow the code!


================
Comment at: llvm/tools/llvm-readobj/ObjDumper.h:162
+  virtual void printLoaderSection(bool PrintHeader, bool PrintSymbolTable,
+                                  bool PrintRelocation) {}
 
----------------
DiggerLin wrote:
> 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"
Your analogy is flawed: "Relocation" and "SymbolTable" are not referring to the same kind of thing.

There is exactly one symbol table, hence why it is singular. The relocation entries consist of one or more relocations: "relocation entry" is just a verbose way of saying "relocation". The term for a group of relocations could be "relocation table" or "relocation section" (see ELF for good examples of this throughout). In other words, "relocation" is the same sort of thing as a "symbol". You could change the `PrintSymbolTable` parameter to `PrintSymbols`, but not to `PrintSymbol` for example.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:331-333
+              LoaderSectionAddr + uintptr_t(LoaderSec->getOffsetToSymTbl()) +
+              (LoaderSecRelEntPtr->SymbolIndex - FirstSymIdxOfLoaderSec) *
+                  sizeof(LoaderSectionSymbolEntry));
----------------
As this expression is getting quite long, it might be worth splitting it into two or three local variables for ease of reading.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:338
+      if (!SymbolNameOrErr) {
+        reportUniqueWarning(SymbolNameOrErr.takeError());
+        return;
----------------
Test case for this warning?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:364
+
+    if (!Obj.is64Bit()) {
+      W.printEnum("Type", (uint8_t)LoaderSecRelEntPtr->Type,
----------------
DiggerLin wrote:
> 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.
FWIW, I don't think it's important to have the order match the data structure order, if there are reasons to do something else. In this case it will simplify code, and also people consuming this won't have to special case whatever scripts they write to handle the order being different.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136787/new/

https://reviews.llvm.org/D136787