[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 a comment.

I wonder if it's worth adding a note the llvm-symbolizer user guide describing the format of the input address? At the moment, it doesn't mention what format the numbers must be in. Probably should be a separate change though, I guess.



================
Comment at: llvm/docs/CommandGuide/llvm-addr2line.rst:21
+-  Requires addresses to be in hexadecimal (with an optional leading ``0x``)
+   instead of interpreting them as C literals.
+
----------------
I don't think referring to it as C literals is necessarily fair, since the style is more widely spread than just C. How about saying "instead of deriving their base from their prefix (if present)"?


================
Comment at: llvm/test/lit.cfg.py:148
 tools.extend([
-    'dsymutil', 'lli', 'lli-child-target', 'llvm-ar', 'llvm-as',
+    'dsymutil', 'lli', 'lli-child-target', 'llvm-ar', 'llvm-as', 'llvm-addr2line',
     'llvm-bcanalyzer', 'llvm-config', 'llvm-cov', 'llvm-cxxdump', 'llvm-cvtres',
----------------
I think this needs reflowing (looks like we're sticking to the 80-column width in this python script).


================
Comment at: llvm/test/tools/llvm-symbolizer/input-base.test:1
+llvm-symbolizer infers the number base from the form of the address.
+RUN: llvm-symbolizer -e /dev/null -a 0x1234 | FileCheck %s
----------------
Could you add a leading '#' character here and for the other comments, to clearly delineate them from the lit and FileCheck directives, please. It makes it easier to read.


================
Comment at: llvm/test/tools/llvm-symbolizer/input-base.test:9
+llvm-addr2line always assumes hexadecimal, but accepts an optional 0x prefix.
+RUN: llvm-addr2line -e /dev/null -a 0x1234 | FileCheck %s
+RUN: llvm-addr2line -e /dev/null -a 1234 | FileCheck %s
----------------
You need a test-case for '0X' prefixes as well as '0x'.


================
Comment at: llvm/test/tools/llvm-symbolizer/input-base.test:13
+RUN: llvm-addr2line -e /dev/null -a 0b1010 | FileCheck %s --check-prefix=HEXADECIMAL-NOT-BINARY
+RUN: llvm-addr2line -e /dev/null -a 0o1234 | FileCheck %s --check-prefix=INVALID-NOT-OCTAL
+
----------------
What gets printed instead here? I thought llvm-addr2line's behaviour was the same a llvm-symbolizer and that it prints the input value as-is if the value is not a valid number, so I'm surprised to see this check passes.

Does reading from /dev/null work on Windows? (I can try it out if you need me to).


================
Comment at: llvm/test/tools/llvm-symbolizer/input-base.test:15-16
+
+CHECK: 0x1234
+HEXADECIMAL-NOT-BINARY: 0xb1010
+INVALID-NOT-OCTAL: 0o1234
----------------
I think you need to add checks for the "name not found pattern" (off the top of my head I want to say '??'), to show that this is actually a look-up and not simply echoing a rejection of a malformed input address.


================
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
----------------
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`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73306