[PATCH] D78549: Improve disassembly of branch targets in non-relocatable objects

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-same-section-addr.test:58
+  - Name:    absolute
+    Index:   0xFFF1
+    Value:   0x0
----------------
SHN_ABS?

Also a suggestion: what about making it:
```
Index: [[INDEX]]
```

And then stop using `# RUN: llvm-objcopy -N absolute %t5 %t6` to remove it,
but also add a test for something else, like `SHN_COMMON` or `SHN_LOPROC` etc?


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-same-section-addr.test:72
+## Two sections, both with symbols, one empty, other not, symbol in non-empty
+## section has too high value.
+# RUN: yaml2obj %s --docnum=2 -o %t10 -D SIZE1=1 -D SIZE2=0 -D SYMVAL1=0x6 -D SYMVAL2=0x5
----------------
Could you expand "too high"?


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-same-section-addr.test:92
+    Address: 0x0
+    Content: e800000000 # Call instruction to next address.
+  - Name:    .first
----------------
`##`


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1256
+           (LHS.first == RHS.first &&
+            LHS.second.getSize() < RHS.second.getSize());
+  });
----------------
Perhaps I'd rewrite this body slightly.
Often sort comparators check one field at once, it is easier to read and mantain them:

```
if (LHS.first != RHS.first)
  return LHS.first < RHS.first;
return LHS.second.getSize() < RHS.second.getSize();
```


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1565
+            for (const SectionSymbolsTy * TargetSymbols : TargetSectionSymbols) {
+                auto Partition = llvm::partition_point(
+                    *TargetSymbols,
----------------
Perhaps just `It`? (consistent with the code above + I guess might look nicer after clang format).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1574
+
+            if(TargetSym != nullptr) {
               uint64_t TargetAddress = TargetSym->Addr;
----------------
Missing a space after `if`. Perhaps 'if (TargetSym)' would be more consistent with the code in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78549