[PATCH] D88076: [llvm-readelf/obj] - Stop printing wrong addresses for arm32 unwind info for non-relocatable objects.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/ARM/unwind-non-relocatable.test:69
+    ContentArray: [
+## Entry 1. Address of .ARM.exidx (0x24C) + entry offset (8) + 0x7fffffe4 == 0x230 (func1).
+                    0xE4, 0xFF, 0xFF, 0x7F, ## Word(0) = 0x7fffffe4.
----------------
psmith wrote:
> entry offset should be (0) for the first entry
Done. I've also updated comments to mention that 31 bit is used in offsets.


================
Comment at: llvm/tools/llvm-readobj/ARMEHABIPrinter.h:547
+    // offset to the referenced entity.
+    const uint64_t Offset =
+        IsRelocatable
----------------
psmith wrote:
> Offset is a bit confusing in this context. As I understand it, it is an offset for relocatable objects but an absolute address for non-relocatable objects.
I think we probably can use `Address` for both? We pass it to the `FunctionAtAddress` below.


================
Comment at: llvm/tools/llvm-readobj/ARMEHABIPrinter.h:549
+        IsRelocatable
+            ? PREL31(Word0, IT->sh_addr)
+            : PREL31(Word0, IT->sh_addr + Entry * IndexTableEntrySize);
----------------
psmith wrote:
> You may want to replace the IT->sh_addr with 0 rather than using the 0 from the relocatable object sh_addr, not a strong opinion though.
> 
> 
Yes, I also found we probably should do it, but I haven't changed it in this patch because it is a possible behavior change for relocatable objects.

If I change this to 0, no our tests fail, but it probably means that the behavior is not well documented by tests. I think changing this for relocatable objects probably belongs to a different patch that should also add a correspondent test.


================
Comment at: llvm/tools/llvm-readobj/ARMEHABIPrinter.h:552
     SW.printHex("FunctionAddress", Offset);
     if (ErrorOr<StringRef> Name = FunctionAtAddress(IT->sh_link, Offset))
       SW.printString("FunctionName", *Name);
----------------
psmith wrote:
> For a non-relocatable ELF file the sh_link field is not reliable as there can be more than one OutputSection, but there will only be one .ARM.exidx OutputSection and the sh_link can only point to one OutputSection. I think that FunctionAtAddress will need changing so that it doesn't require a matching section when there is non-relocatable objects.
> 
> 
Thanks! I've did this change and updated the test case to drop `sh_link` field of the `SHT_ARM_EXIDX` section.


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

https://reviews.llvm.org/D88076