[PATCH] D73306: llvm-addr2line: assume addresses on the command line are hexadecimal rather than attempting to guess the base based on the form of the number.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-addr2line.rst:25-37
+-  ``llvm-addr2line`` defaults not to print function names. Use `-f`_ to enable
+   that.
 
--  Defaults not to print inlined frames. Use `-i`_ to show inlined
-   frames for a source code location in an inlined function.
+-  ``llvm-addr2line`` defaults not to demangle function names. Use `-C`_ to
+   switch the demangling on.
 
+-  ``llvm-addr2line`` defaults not to print inlined frames. Use `-i`_ to show
----------------
The rewording changes look good to me, but please put them in a separate commit (feel free to push that bit with no further review).


================
Comment at: llvm/test/tools/llvm-symbolizer/input-base.test:19
+RUN: llvm-addr2line -e /dev/null -a 0o1234 | FileCheck %s --check-prefix=INVALID-NOT-OCTAL
+RUN: llvm-addr2line -e /dev/null -a 0O1234 | FileCheck %s --check-prefix=INVALID-NOT-OCTAL
+
----------------
This line working is surprising to me. FileCheck is supposed to be case sensitive, so it looks to me like llvm-addr2line is consuming `0O1234` as an actual number or something and not treating it as an invalid address.


================
Comment at: llvm/test/tools/llvm-symbolizer/input-base.test:21
+
+CHECK: 0x1234
+HEXADECIMAL-NOT-BINARY: 0xb1010
----------------
Some of my comments have moved around too much in Phabricator's view to easily see the context, so apologies if this feels like a bit of repetition. This CHECK on its own is insufficient for the cases where the input is the string `0x1234`. In those cases, this CHECK will not distinguish between llvm-addr2line recognising the string as a valid hexadecimal address (and thus being able to use it to do the look up) and not (and thus just echoing it to the output). I think the only ways to tell are to either turn off `-a` (which would cause valid addresses to not appear and invalid values to be printed), or to check for the not-found pattern (to prove that a lookup was performed). I'd recommend the latter.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:223
   int offset_length = strcspn(pos, kDelimiters);
-  return !StringRef(pos, offset_length).getAsInteger(0, ModuleOffset);
+  StringRef offset(pos, offset_length);
+  // GNU addr2line assumes the offset is hexadecimal and allows a redundant
----------------
rsmith wrote:
> jhenderson wrote:
> > I do not like the mixed variable naming styles in this function!
> > 
> > Given we use both upper and lower-case variable names, let's make this one follow the standard LLVM style (i.e. `Offset`).
> Every local variable in this function uses `lower_camel_case`, so I followed that. I'll just change them all to follow LLVM convention (as a separate commit).
Could you do the variable renaming in a separate pre-requisite patch, please, and then rebase this patch on top of that? That'll make it easier to spot the real differences.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73306