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

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