[PATCH] D148049: [llvm-objdump] Add --markup-context to adjust VMAs.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:185
+  Adjust addresses using a JSON context file produced from llvm-symbolizer
+  markup (i.e., llvm-symbolizer --dump-context). Note that llvm-symbolizer can
+  emit multiple contexts as an array of JSON objects, but this flag accepts
----------------
This reference to the llvm-symbolizer option should be a link to the relevant documentation.


================
Comment at: llvm/test/tools/llvm-objdump/X86/markup-context.test:10
+# OUTPUT-NEXT:   Idx Name           Size     VMA              Type
+# OUTPUT-NEXT:    0                 00000000 0000000000000000
+# OUTPUT-NEXT:    1 .text           00000002 0000000000123000 TEXT
----------------
In this and other dumps below, since it's really only the VMA that you care about, you should omit the other columns (replace with wildcards where necessary), to make it clear what you're actually trying to test.


================
Comment at: llvm/test/tools/llvm-objdump/X86/markup-context.test:48
+
+# MISSING-FILE: error: '{{.*}}missing.json': No such file or directory
+# INVALID-JSON: error: '{{.*}}invalid.json': [2:0, byte=2]: Expected object key
----------------
This will fail on various OSes. You should use the %errc... substitutions to get the platform-specific message. You'll need to pass them in as FileCheck variables.


================
Comment at: llvm/test/tools/llvm-objdump/X86/markup-context.test:59
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
----------------
I think it would be useful to have comments in the YAML to explain why each section/symbol etc is interesting to the test.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:3054
   MachOOpt = InputArgs.hasArg(OBJDUMP_macho);
+  if (const opt::Arg *A = InputArgs.getLastArg(OBJDUMP_markup_context_EQ)) {
+    StringRef Filename = A->getValue();
----------------
You should probably have a test case that shows that only the last --markup-context is used.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:3058
+        MemoryBuffer::getFileOrSTDIN(Filename);
+    if (!Buf)
+      reportError(Filename, Buf.getError().message());
----------------
There are three checks here, but only two test cases that I can map them too. Is one of them missing a test case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148049