[PATCH] D57051: [llvm-objdump] - Implement the --adjust-vma option.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 23 07:00:32 PST 2019
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
More information about the llvm-commits
mailing list