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

Tue Jan 31 00:57:12 PST 2023

jhenderson added a comment.

In D139097#4073914 <https://reviews.llvm.org/D139097#4073914>, @peter.smith wrote:

> So I think this still leaves us with the choice of whether llvm-nm should differ from arm-none-eabi-nm. I personally would err on the side of caution and consistency between GNU and LLVM and keep the default behaviour the same. However I don't think it will make a huge difference.

I'm inclined to agree with @peter.smith re. compatibility. I don't think there's a strong enough motivation to diverge here, and I've seen llvm-nm used in scripts before to gather symbol data. This means we can't safely change the behaviour like we can with some other tools that are likely only for direct human consumption.

I think this means the option has a place here. @MaskRay, any further thoughts?

On the note of this new option, it should be in the llvm-nm CommandGuide documentation. Also, given you specifically call out MIPS in the comments etc, it probably makes sense to have testing for MIPS too?

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

================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:614-615
+    Result = getCommonSymbolSize(Symb);
+  else {
+    if (!SymOrErr)
+      // TODO: Test this error.
----------------
This code flow looks a bit clunky and easy to misread. Could this become an `else if` with the above, and then the bit following this `if` can become the `else`?

================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:616
+    if (!SymOrErr)
+      // TODO: Test this error.
+      return SymOrErr.takeError();
----------------
As above: the new code path should be tested.

================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:627
+  if (!SectionAddressOrError)
+    return SectionAddressOrError.takeError();
+
----------------
quic-subhkedi wrote:
> jhenderson wrote:
> > peter.smith wrote:
> > > The code base has many  `// TODO: Test this error.` comments, is it worth replicating them here as this is the same code pattern?
> > > 
> > > Not a strong opinion. 
> > Some context: these were added by @grimar some time ago. Ideally, all our code paths should have at least one test that exercises that code path, so in this case, we'd want a test case that shows what happens when there's an error fetching the address. The TODOs were added because at the time, it either was tricky or completely impossible to craft a test case, given the tools we have. That might not be the case here (but if it is, a TODO may be appropriate).
> yes it had to replicated
Sorry for the confusion: new code should be tested. The existing TODOs are for code paths that weren't properly tested previously and which @grimar didn't have time to identify a way to test.

You should be able to write a test using yaml2obj has many ways to craft an object with malformed input. If there's literally no way to hit this code path (even if you were to hand-edit a binary), then a return of the error is inappropriate and it should be `cantFail`.

================
Comment at: llvm/test/tools/llvm-nm/ARM/address-check.test:1-2
+## Test for function type symbols with odd valued address to reflect raw value
+## on ARM when --print-raw-values is used
+#
----------------
I think this comment should be reworded. Also, as this test is in the ARM subdirectory, there's no need to specifically say that this is for ARM here.

================
Comment at: llvm/test/tools/llvm-nm/ARM/address-check.test:3
+## on ARM when --print-raw-values is used
+#
+# RUN: yaml2obj %s -o %t
----------------
Nit: We don't usually have these extra lines between comments and test logic for simple tests with a single test case, like this test.

================
Comment at: llvm/test/tools/llvm-nm/ARM/address-check.test:5-6
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm %t | FileCheck %s
+# RUN: llvm-nm %t --print-raw-values | FileCheck %s --check-prefix=CHECKRAW
+
----------------
Rather than two prefixes, I think it would be better to use a single test prefix, but using FileCheck's -D to specify the expected value. That way, there's no risk of another output change causing one test case to pass and the other to no longer be covered for some reason. I'd also move the thing being checked for close to the RUN line, as it's short.

Something like the following:
```
# RUN: llvm-nm %t | FileCheck %s -DVALUE=00001002
# RUN: llvm-nm %t --print-raw-values | FileCheck %s -DVALUE=00001003

# CHECK: [[VALUE]] T foo
```

================
Comment at: llvm/test/tools/llvm-nm/ARM/address-sort.test:1-2
+## Test for function type symbols with odd valued address to reflect raw value
+## on ARM when --print-raw-values is used along with --numeric-sort
+#
----------------
It's not clear from this comment why --numeric-sort + --print-raw-values is an interesting combination to test. Could you rephrase the comment (also including my suggestions from the other test case) to explain why this combination is important. Looking at the code, I think it's because the address used in the sort the printed address (i.e. the "raw" one when `--print-raw-values` is used), so you should say something about that.

================
Comment at: llvm/test/tools/llvm-nm/ARM/address-sort.test:32-35
+# CHECK-DAG: 00001002 T foo
+# CHECK-DAG: 00001002 T bar
+# CHECKRAW-DAG: 00001003 T foo
+# CHECKRAW-DAG: 00001002 T bar
----------------
CHECK-DAG means that the output can be in either order between the two check lines. Is that what you actually want?

================
Comment at: llvm/tools/llvm-nm/Opts.td:59
+// ARM and microMIPS specific option.
+def print_raw_values:  FF<"print-raw-values", "Print raw values for symbols">;
+
----------------
The help text probably should mention the ARM/MIPS specificness of this option.

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

https://reviews.llvm.org/D139097