[PATCH] D105985: Support GSYM in llvm-symbolizer.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I have zero experience with or knowledge about gsym, so I can't comment on the implementation details, but some higher-level comments:

1. Make sure to review the llvm-symbolizer documentation to see if there's anything there that needs adding/updating given this change (there may not be).
2. Can you avoid using pre-canned binaries for the test input, by somehow generating the inputs on the fly? The existing pre-canned binaries in the llvm-symbolizer tests are already sub-optimal, in my opinion, as they are opaque and make testing less flexible.



================
Comment at: llvm/test/tools/llvm-symbolizer/sym-gsymonly.test:1
+#Source:
+##include <stdio.h>
----------------
Probably worth a top-level comment explaining what this test is supposed to be testing.

Also probably you can just rename this test "gsym.test". I'm not sure what the "only" bit is about, and the "sym-" prefix similarly doesn't look to add any meaning.


================
Comment at: llvm/test/tools/llvm-symbolizer/sym-gsymonly.test:20
+
+RUN: llvm-symbolizer -print-address -obj=%p/Inputs/addr-gsymonly.exe < %p/Inputs/addr.inp | FileCheck %s
+RUN: llvm-symbolizer -addresses -obj=%p/Inputs/addr-gsymonly.exe < %p/Inputs/addr.inp | FileCheck %s
----------------
Nit: I'd normalise the comment markers throughout this test, using the following two rules:

1) True comments start with `##` (followed by a space). Applies for the source above. This helps distinguish the test details from actual comments.
2) Add `#` as a comment marker to all RUN lines, much as we do in the vast majority of newer tests. Follow it with a space again.
3) CHECK and equivalent lines should be `# CHECK` (with space).

Finally, as noted out-of-line, I've not looked at the implementation, but do you need all these individual test-cases for gsym testing? Most of them look more like they're testing generic symbolizer behaviour, which is therefore already covered elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105985