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

Peter Smith via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 6 12:03:45 PST 2022


peter.smith added a comment.

I don't think adding state to the shared llvm/Object/ELFObjectFile.h is the right thing to do there. I think that bit of code is used in too many places that are relying on the behaviour being what it is without having it mutated by a stateful parameter. I think you could add an additional member function that retrieved the symbol without any additional processing, I think this would show the intent more clearly, as well as not adding state. llvm-nm could call that function when wanting raw symbol addresses.

Alternatively you could use the mapping symbols $t and $a along with the symbol type to identify the Thumb code symbols and add the Thumb bit back in llvm-nm. That wouldn't work for MicroMips, but I think you care about Arm.

I expect that llvm-nm and arm-none-eabi-nm use the address and not the raw value so that `--numeric-sort` gives the right answer. Otherwise mapping symbols, and any other `STT_NOTYPE` symbol aliases will be at different addresses to the Thumb function symbols themselves.

I personally don't have any objections to adding functionality to llvm-nm. I do think it needs to avoid altering code-paths that depend on the function the way that it is without introducing state though. I can't comment on whether this change is worth the additional complexity, will need to run this by a maintainer to get the final decision on that.



================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:64
 protected:
+  bool ShowRawValue = false;
+
----------------
I don't think that this is a good idea to introduce state in such a general place. This header is used for all sorts of tools, they will be expecting that the Thumb bit and the Micromips indicator bit are stripped. If this is ever set by another user then all sorts of things could break.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:565
 
+  // If raw value bool is set then emit raw value
+  if (getShowRawValue())
----------------
I'm not a fan of altering the behaviour of this function with state. I would prefer a new function that gets the raw symbol value. I think that shows the intent of the change rather than mutating an existing function that is in widespread use.


================
Comment at: llvm/tools/llvm-nm/Opts.td:85
+
+// ARM specific option
+def print_raw_values:  FF<"print-raw-values", "Print raw values for symbols">, Flags<[HelpHidden]>;
----------------
Also for Mips, see comment below.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:133
 
+// ARM specific option.
+static bool PrintRawValues = false;
----------------
Also for Mips. The microMIPS ISA uses the bottom bit for the same purpose as Thumb. See ISA Mode Switch in the MIPS32 Architecture Reference Manual.


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

https://reviews.llvm.org/D139097



More information about the llvm-commits mailing list