[PATCH] D57051: [llvm-objdump] - Implement the --adjust-vma option.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm a little confused as to what the purpose of this option is? I think it can be used to produce disassembly that can then be grepped when loading at a variable address (e.g. due to ASLR), but am wondering if that's the intent?

> I am adjusting sections that are not allocatable. As, for example, adjusting debug sections VA's and rel[a] sections VA's should not make sense.

Do you mean you are NOT adjusting non-alloc sections (i.e. what your patch actually does)?



================
Comment at: test/tools/llvm-objdump/X86/adjust-vma.test:33
+# COMMON-NEXT:     0: {{.*}}  addl %eax, (%rax)
+## ... There a more lines here. We do not care.
+
----------------
a -> are


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:74
+cl::opt<unsigned long long>
+    AdjustVma("adjust-vma",
+              cl::desc("Offset the addressed displayed by a value"),
----------------
AdjustVma -> AdjustVMA (since it's an abbreviation)


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:75
+    AdjustVma("adjust-vma",
+              cl::desc("Offset the addressed displayed by a value"),
+              cl::value_desc("offset"), cl::init(0));
----------------
I'd rephrase this as "Increase the displayed address by the specified offset"


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:878
+// specified value for a given section.
+// For ELF we do not adjust the non-allocatable sections like debug ones,
+// because they are not loadable.
----------------
adjust the non-allocatable -> adjust non-allocatable


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:880
+// because they are not loadable.
+// TODO: implement for other targets.
+static bool shouldAdjustVA(const SectionRef &Section) {
----------------
targets -> file formats


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1048-1050
+    uint64_t AddVMA = 0;
+    if (shouldAdjustVA(Section))
+      AddVMA = AdjustVma;
----------------
This seems to be done rather early given its use-site. Could it be moved closer?

I'm not a fan of the variable name, as it isn't really a noun. Perhaps `VMAAdjustment` would be better?


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:1433
+            
+            // When --adjust-vma is used update the address printed.
+            if (RelCur->getSymbol() != Obj->symbol_end()) {
----------------
used update -> used, update


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

https://reviews.llvm.org/D57051