[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 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
----------------
jhenderson wrote:
> 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).
They are already in a separate commit; sorry for not mentioning that. (FYI, you can see the commits in the "Commits" tab in phabricator.) I went ahead and committed that.


================
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
+
----------------
jhenderson wrote:
> 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.
Nice catch =) Sorry, I made this change at the last minute before uploading and forgot to rerun the test, which does indeed fail as you expected. And it turns out that `llvm-symbolizer` doesn't accept the `0O` (zero, capital o) prefix either. Test updated.


================
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:
> 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.
This was already split out into a separate commit, but... it doesn't look like Phabricator lets you look at the contents of the individual commits in a review. That seems to make the ability to upload a series of commits to Phabricator as a single review... kind of useless. I've gone ahead and submitted the renaming since I think it's obvious and uncontroversial.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73306