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

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith marked 2 inline comments as done.
rsmith added inline comments.


================
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.
+
----------------
jhenderson wrote:
> 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)"?
The major difference I was trying to get across is that unprefixed numbers default to decimal in `llvm-symbolizer` (or octal if there's a `0` prefix) but to hexadecimal in `llvm-addr2line`, and I don't think your alternative really captures that. I've taken another pass over this. (I also changed the bullets to include a noun, since it wasn't really clear whether these bullets were talking about the behavior of `addr2line` or `symbolizer`.)


================
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
----------------
jhenderson wrote:
> You need a test-case for '0X' prefixes as well as '0x'.
Added both cases for all the tests.


================
Comment at: llvm/test/tools/llvm-symbolizer/input-base.test:15-16
+
+CHECK: 0x1234
+HEXADECIMAL-NOT-BINARY: 0xb1010
+INVALID-NOT-OCTAL: 0o1234
----------------
jhenderson wrote:
> 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.
Note the "x". That wasn't there in the 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
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73306