[PATCH] D139097: [ARM] Add option --print-raw-value to llvm-nm to dump raw symbol values in case of ARM

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Please add the new option to the llvm-nm CommandGuide documentation.

I was under the impression this patch was to print the raw symbol value, which would be the raw st_value field of the Elf_Sym object. However, this function isn't doing that. Could you explain why, please?



================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:594
 
+  auto SectionAddressOrError = getSectionAddressFromSymbol(Symb);
+  if (!SectionAddressOrError)
----------------
Nit: I'm not sure `auto` is appropriate here. LLVM doesn't have an always-auto policy, and I can't tell from here or the immediate context what the underlying type is.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:604
+Expected<uint64_t>
+ELFObjectFile<ELFT>::getRawSymbolAddress(DataRefImpl Symb) const {
+  uint64_t Result;
----------------
Some of the code in this function is duplicated with `getSymbolAddress`. Indeed, I'd expect the two functions (based on their names) to do almost the same thing, with `getSymbolAddress` simply doing the additional masking. That doesn't seem to be the case though.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:596-597
+  if (!SectionAddressOrError)
+    // TODO: Test this error.
+    return SectionAddressOrError.takeError();
+
----------------
jhenderson wrote:
> As this is a new code path, rather than a pre-existing one, it should have a test that covers it.
If this is now tested, please remove the TODO.


================
Comment at: llvm/test/tools/llvm-nm/ARM/address-check.test:1
+## Test --print-raw-values flag to reflect raw value of symbols.
+# RUN: yaml2obj %s -o %t
----------------



================
Comment at: llvm/test/tools/llvm-nm/ARM/address-check.test:1
+## Test --print-raw-values flag to reflect raw value of symbols.
+# RUN: yaml2obj %s -o %t
----------------
jhenderson wrote:
> 
This test name makes no sense. What does "address-check" have to do with "print-raw-values".

I recommend changing the name to something like "print-raw-values.test".


================
Comment at: llvm/test/tools/llvm-nm/ARM/address-check.test:14
+  Machine:         EM_ARM
+  Flags:           [ EF_ARM_EABI_VER5 ]
+Sections:
----------------
Is the flag actually needed for this test? If not, please delete this line to minimise noise.


================
Comment at: llvm/test/tools/llvm-nm/ARM/address-check.test:19
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x4
+Symbols:
----------------
This is unnecessary for the test case, I believe.


================
Comment at: llvm/test/tools/llvm-nm/ARM/address-check.test:25
+    Type:    STT_FUNC
+    Binding: STB_GLOBAL
----------------
You can omit the Binding field. It'll cause the letter code llvm-nm prints to change, but this is irrelevant for the test.


================
Comment at: llvm/test/tools/llvm-nm/ARM/address-sort.test:1-2
+## Test --print-raw-values flag with --numeric-sort and check the symbol
+## ordering is according to raw value of the symbols.
+# RUN: yaml2obj %s -o %t
----------------
Some wording tweak suggestions.


================
Comment at: llvm/test/tools/llvm-nm/ARM/address-sort.test:18
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x4
+Symbols:
----------------
Same comments about minmising the YAML as in the other test case.


================
Comment at: llvm/test/tools/llvm-nm/ARM/address-sort.test:31
+
+
+# CHECK: 00001002 T bar
----------------
Nit: delete extra blank line.


================
Comment at: llvm/test/tools/llvm-nm/ARM/address-sort.test:32-35
+# CHECK: 00001002 T bar
+# CHECK: 00001002 T foo
+# CHECKRAW: 00001002 T bar
+# CHECKRAW: 00001003 T foo
----------------
As with the first test, I find it easier to follow tests if the check lines are before the YAML.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1840-1841
+            PrintRawValues && ELFObj
+                ? ELFObj->getRawSymbolAddress(Sym.getRawDataRefImpl())
+                : SymRef.getAddress();
         if (!AddressOrErr) {
----------------
This asymmetry makes me think that `getRawSymbolAddress` should be `getRawAddress` and be a metho of `SymbolRef`.

Also, is this behaviour ELF-specific or does e.g. COFF have the same behaviour, and therefore the option should apply there too?


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

https://reviews.llvm.org/D139097