[PATCH] D74819: [DebugInfo] Report unsupported maximum_operations_per_instruction values

Thu Mar 5 06:01:58 PST 2020

jhenderson marked an inline comment as done.
jhenderson added inline comments.

================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:374
+    uint64_t advanceAddr(uint64_t OperationAdvance, uint64_t OpcodeOffset,
+                         StringRef OpcodeName);

----------------
probinson wrote:
> As a future API simplification, think about using `dwarf::LNStandardName()` to compute the names as needed, instead of passing around strings.  LNStandardName() would need an opcode_base parameter to know which ones are special for a particular line-number program, but it's used in only one place that I can find, so that should be an easy update.
> I know there's the weird case of DW_LNS_const_add_pc where you're basing the advance on 255 but want to use the real opcode for reporting; but the weird cases are why we pay you the big bucks.
I had a play around with this, and LNStandardName isnt't suitable for passing in opcode_base with directly, as it needs to share the interface with the equivalent functions for other DWARF enum types. I can workaround this with a little extra logic though, I think.

================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:9

+#include "llvm/DebugInfo/DWARF/DWARFDebugLine.h"
 #include "DwarfGenerator.h"
----------------
probinson wrote:
> jhenderson wrote:
> > probinson wrote:
> > > Should not have moved this; it's not the header for this module.
> > Weird. I don't know how that happened. I certainly wouldn't have deliberately moved it.
> Did it move again?  If you ran clang-format, I wonder if there's a bug in it (it might think it's moving the "module's" header to the top, but have incorrect module-header detection).
Weird...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74819