[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 subscriber: grimar.
jhenderson added a comment.

A few nits, but unfortunately, as today is my last day working for 6 weeks, I won't be able to review further at this time. If @MaskRay or another developer hasn't had a chance to look at this, I'll look when I get back end of January (please ping me at that point, as I may forget otherwise).



================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:604
+ELFObjectFile<ELFT>::getRawSymbolAddress(DataRefImpl Symb) const {
+
+  uint64_t Result;
----------------
Nit: delete this blank line.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:627
+  if (!SectionAddressOrError)
+    return SectionAddressOrError.takeError();
+
----------------
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).


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:635
+ELFObjectFile<ELFT>::getSectionAddressFromSymbol(DataRefImpl Symb) const {
+
   auto SymTabOrErr = EF.getSection(Symb.d.a);
----------------
Nit: delete this blank line.


================
Comment at: llvm/tools/llvm-nm/Opts.td:86
+// ARM and microMIPS specific option.
+def print_raw_values:  FF<"print-raw-values", "Print raw values for symbols">, Flags<[HelpHidden]>;
----------------
What's the motivation for hiding this option?


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

https://reviews.llvm.org/D139097