[PATCH] D70720: [llvm-objdump] Display locations of variables alongside disassembly
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 5 03:34:34 PST 2019
jhenderson added a comment.
I've mostly focused on style comments in this review round.
================
Comment at: llvm/lib/Support/FormattedStream.cpp:31-32
for (const char *End = Ptr + Size; Ptr != End; ++Ptr) {
+ // If this is a multi-byte sequence, skip the extra bytes, and don't check
+ // for special whitespace characters.
+ if ((*Ptr & 0b11100000) == 0b11000000) {
----------------
ostannard wrote:
> jhenderson wrote:
> > What do you mean by multi-byte here?
> UTF-8 characters which are represented by 2, 3 or 4 bytes. This is need for `getColumn` and `PadToColumn` to work correctly with the unicode line-drawing characters.
Okay. I'd suggest changing the phrase "multi-byte sequence" to "multi-byte UTF-8 character", as the current phrasing could be confused with Windows multi-byte characters to people from that background.
================
Comment at: llvm/test/tools/llvm-objdump/ARM/debug-vars-dwarf4-sections.s:1-9
+## RUN: llvm-mc -triple armv8a--none-eabi < %s -filetype=obj | \
+## RUN: llvm-objdump - -d -debug-vars -no-show-raw-insn | \
+## RUN: FileCheck %s
+
+# Check that the -debug-vars option works for simple register locations, when
+# using DWARF4 debug info, with functions im multiple sections.
+
----------------
I think you misread my comment. Use '##' for comments and '#' for test directives. I'd also put the comments at the top of the test file:
```
## Comments
...
# RUN:
...
# CHECK:
...
<assembly>
```
================
Comment at: llvm/test/tools/llvm-objdump/ARM/debug-vars-dwarf4-sections.s:2
+## RUN: llvm-mc -triple armv8a--none-eabi < %s -filetype=obj | \
+## RUN: llvm-objdump - -d -debug-vars -no-show-raw-insn | \
+## RUN: FileCheck %s
----------------
This probably isn't very consistent in some tests, but prefer "--" to "-" for long-options, especially for the GNU-equivalent binutils where "-" can lead to ambiguous switches (e.g. is "-abc" to be interpreted as "-a", "-b" and "-c" or "--abc"?).
================
Comment at: llvm/test/tools/llvm-objdump/ARM/debug-vars-dwarf4-sections.s:3
+## RUN: llvm-objdump - -d -debug-vars -no-show-raw-insn | \
+## RUN: FileCheck %s
+
----------------
Consider using --strict-whitespace for this and the other tests. If you think it is unnecessary, then you can treat any contiguous block of whitespace in the CHECK lines as one (or more) space(s), and you can therefore shorten the lines.
================
Comment at: llvm/test/tools/llvm-objdump/ARM/debug-vars-dwarf4-sections.s:57
+.Lfunc_begin0:
+ .file 1 "/work" "llvm/src/llvm/test/tools/llvm-objdump/ARM/Inputs/debug.c"
+ .loc 1 1 0
----------------
Is this string important to the test? If it is, I don't think it'll work on others' machines.
You may need to use sed to replace the input string at test time. There are examples in the source-interleave tests in the X86 folder of llvm-objdump.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:346-350
+static cl::opt<int>
+ DbgIndent("debug-vars-indent", cl::init(50),
+ cl::desc("Distance to indent the source-level variable display, "
+ "relative to the start of the disassembly"),
+ cl::cat(ObjdumpCat));
----------------
I don't see any testing for this option?
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:571
+
+static void PrettyPrintDWARFOps(raw_ostream &OS, DWARFExpression::iterator I,
+ const DWARFExpression::iterator E,
----------------
Function names should start with a lower-case letter, i.e. `prettyPrintDWARFOps`. Applies elsewhere too.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:577
+ while (I != E) {
+ auto &Op = *I;
+ unsigned Opcode = Op.getCode();
----------------
Too much `auto`
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:578
+ auto &Op = *I;
+ unsigned Opcode = Op.getCode();
+ switch (Opcode) {
----------------
You can make this `uint8_t` to match the return type of `getCode()`
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:581
+ case dwarf::DW_OP_regx: {
+ int DwarfRegNum = Op.getRawOperand(0);
+ int LLVMRegNum = *MRI->getLLVMRegNum(DwarfRegNum, false);
----------------
Why is this an `int`? `getRawOperand` returns a uint64_t.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:582
+ int DwarfRegNum = Op.getRawOperand(0);
+ int LLVMRegNum = *MRI->getLLVMRegNum(DwarfRegNum, false);
+ raw_svector_ostream S(Stack.emplace_back(PrintedExpr::Value).String);
----------------
`getLLVMRegNum` returns an `Optional<unsigned>`. Where is the checking to make sure it's safe to dereference it? Why is this converting to an `int`?
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:589-590
+ if (Opcode >= dwarf::DW_OP_reg0 && Opcode <= dwarf::DW_OP_reg31) {
+ int DwarfRegNum = Opcode - dwarf::DW_OP_reg0;
+ int LLVMRegNum = *MRI->getLLVMRegNum(DwarfRegNum, false);
+ raw_svector_ostream S(Stack.emplace_back(PrintedExpr::Value).String);
----------------
Why `int`?
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:594
+ } else {
+ // If we hit an unknown operand, we don't know it's effect on the stack,
+ // so bail out on the whole expression.
----------------
it's -> its (it's == it is; its == belonging to it)
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:665
+
+ // Coulmns which we are currently drawing.
+ IndexedMap<Column> ActiveCols;
----------------
Coulmns -> Columns
I'd also rephrase slightly: "The columns we are currently drawing."
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:680
+ if (!Locs) {
+ consumeError(Locs.takeError());
+ return;
----------------
Why are you throwing away the error and not report an error or warning?
Add a comment if there's a valid reason not to report a message.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:684
+
+ for (auto &LocExpr : *Locs) {
+ if (LocExpr.Range) {
----------------
Please don't use `auto` here. I don't know what the type is. Also, is this able to be a `const &`?
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:686-688
+ // FIXME: getLocations seems to get the section index wrong for
+ // objects built with -ffunction-sections, for now we just fix it up
+ // here.
----------------
File a bug for this FIXME against the getLocations method.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:708
+ void AddFunction(DWARFDie D) {
+ for (auto Child : D.children()) {
+ if (Child.getTag() == dwarf::DW_TAG_variable ||
----------------
Too much `auto`. Should it be `const &`?
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:729-730
+ unsigned MoveToFirstVarColumn(formatted_raw_ostream &OS) {
+ unsigned FirstUnprintedColumn =
+ std::max((int)(OS.getColumn() - getIndentLevel() + 1) / 2, 0);
+ if ((getIndentLevel() + FirstUnprintedColumn * 2) > OS.getColumn())
----------------
What's the `/ 2` and `* 2` below about?
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:746
+
+
+public:
----------------
Too many blank lines. Have you clang-formatted?
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:750
+ LiveVariablePrinter(const MCRegisterInfo *MRI, bool LittleEndian)
+ : LiveVariables(), ActiveCols({~0U, false, false, false}), MRI(MRI),
+ LittleEndian(LittleEndian) {}
----------------
I'd pull `~0U` into a constant and/or use `std::numeric_limits::max()`.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:765
+ else
+ for (auto Child : D.children())
+ AddFunction(Child);
----------------
Too much `auto`. Should it be `const &`?
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:780
+ SmallSet<unsigned, 8> CheckedVarIdxs;
+ for (unsigned ColIdx = 0; ColIdx < ActiveCols.size(); ++ColIdx) {
+ if (!ActiveCols[ColIdx].isActive())
----------------
[[ https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop | Canonical LLVM style is to precalculate the end value ]] rather than on every iteration. Also applies below.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1076
+
class PrettyPrinter {
----------------
Delete extra blank line.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1603
+ DICtx = DWARFContext::create(*Obj);
+ for (const auto &CU : DICtx->compile_units())
+ LVP.AddCompileUnit(CU->getUnitDIE(false));
----------------
Too much `auto`. What is the type here?
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:551
namespace {
+struct PrintedExpr {
+ enum ExprKind {
----------------
aprantl wrote:
> doxygen comments explaining the purpose of this?
This and the other places you've suggested this are anonymous namespace/static local functions in a .cpp file. I don't think it needs doxygen style comments. Also, doxygen files aren't generated for items in the tools directory.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1064
+ // with box drawing characters joining it to the live range line.
+ OS << (ActiveCols[ColIdx].LiveIn ? "┠─ " : "┌─ ");
+ WithColor(OS, raw_ostream::GREEN)
----------------
ostannard wrote:
> jhenderson wrote:
> > As noted on the mailing list, have you checked that this works with typical Windows command prompts (i.e. cmd and PowerShell)? Can plain ASCII serve your purpose just as easily?
> I've not tried this on windows yet, I'll do that today. I did experiment with some plain ascii versions, but none of them were as clear as the unicode version, so I don't think we should switch to ascii everywhere, even if we do need it for windows.
It may be worth providing a "style" argument to the option that is able to specify a drawing style i.e. "ascii" or "box" or something like that. The formatting strings could then be configured based on that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70720/new/
https://reviews.llvm.org/D70720
More information about the llvm-commits
mailing list