[PATCH] D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 9 01:02:50 PST 2020
jhenderson added a comment.
There's at least one pre-merge bot failure that looks like its failing due to something in this change.
I've not reviewed the testing yet (but am happy to do so if I get a chance in the coming days), but I've done a mostly cosmetic review of the rest. I don't have enough experience in this area to review the detail though.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:41
+ Undefined,
+ /// Register is unchanged and in the register itself:
+ /// reg = reg
----------------
This comment is a little confusing to me. The register is in itself? Should it be "Value is unchanged..."? Note: I don't know much about this stuff, so accept that might not make sense.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:88
+ /// Create a location whose rule is set to Unspecified. This means the
+ /// register value might be in the same registers but it wasn't specified in
+ /// the unwind opcodes.
----------------
Should this be `register` rather than `registers`?
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:97
+ /// Create a location that is in (Deref == false) or at (Deref == true) the
+ /// CFA plus an offset. Most registers that are spilled into the stack use
+ /// this rule. The rule for the register will use this rule and specify a
----------------
I think you spill onto the stack, not into it, right?
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:163
+///
+/// The register maps is put into a class so that all register locations can be
+/// copied when parsing the unwind opcodes DW_CFA_remember_state and
----------------
(or "maps are")
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:195
+
+ /// Removes any rule for the register in \a RegNum
+ ///
----------------
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:200
+
+ /// Dump all register + locations that are currently defined in this object.
+ ///
----------------
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:228
+///
+/// The row consists of a optional address, the rule to unwind the CFA and all
+/// rules to unwind any registers. If the address doesn't have a value, this
----------------
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:273
+ void slideAddress(uint64_t Offset) {
+ assert(Address.hasValue());
+ *Address += Offset;
----------------
Unnecessary assert?
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:305
+
+/// A class that contains all UnwindRow objects for a FDE or a single unwind
+/// row for a CIE. To unwind an address the rows, which are sorted by start
----------------
Maybe. I say "an eff-dee-ee" in my head for this.
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:343
+
+ /// Create a UnwindTable from a Common Information Entry (CIE).
+ ///
----------------
================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:353
+
+ /// Create a UnwindTable from a Frame Descriptor Entry (FDE).
+ ///
----------------
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:367
+ if (OperandIdx >= 2)
+ return createStringError(std::errc::invalid_argument,
+ "Operand index %u is not valid.", OperandIdx);
----------------
There's inconsistency here and above. Earlier you use `errc::invalid_argument` without the `std`. We should be consistent!
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:368
+ return createStringError(std::errc::invalid_argument,
+ "Operand index %u is not valid.", OperandIdx);
+ OperandType Type = CFIP.getOperandTypes()[Opcode][OperandIdx];
----------------
These messages should all use `PRIu32` to go with the `uint32_t` type.
Also, the style for error messages according to the style guide nowadays is no capitalization on first letter (for regular words at least) and no trailing full stop.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:428
+ return createStringError(std::errc::invalid_argument,
+ "Operand index %u is not valid.", OperandIdx);
+ OperandType Type = CFIP.getOperandTypes()[Opcode][OperandIdx];
----------------
Same comment as above re. error message style.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:432
+ switch (Type) {
+ case OT_Unset:
+ return createStringError(std::errc::invalid_argument,
----------------
Some of these cases are identical. Can we combine them? Same goes above in `getOperandAsUnsigned`.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:498
+ std::vector<RegisterLocations> RegisterStates;
+ for (const auto &Inst : CFIP) {
+ switch (Inst.Opcode) {
----------------
Maybe not use `auto` here (what is the type of `Inst`?)
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:508
+ // location is preceded by a segment selector of the given length
+ auto NewAddress = Inst.getOperandAsUnsigned(CFIP, 0);
+ if (!NewAddress)
----------------
This probably shouldn't be `auto`. What is the size of the return value? Same goes below for `Offset`, `RegNum` etc.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:612
+ // should be initially set to 1.
+ constexpr uint32_t AARCH64_DWARF_PAUTH_RA_STATE = 34;
+ auto LRLoc = Row.getRegisterLocations().getRegisterLocation(
----------------
Not sure whether this is a clang-tidy issue or something not matching coding standards here. If the former, perhaps worth reporting it? If the latter, please fix!
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:643
+ default: {
+ std::string ArchName = Triple::getArchTypeName(CFIP.triple()).str();
+ return createStringError(errc::not_supported,
----------------
Any particular reason to do the `str()` here rather than just doing `.str().c_str()` below?
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:687
+
+ case dwarf::DW_CFA_val_offset: {
+ auto RegNum = Inst.getOperandAsUnsigned(CFIP, 0);
----------------
This seems to be identical to the below case? Could they be folded together.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:741
+ case dwarf::DW_CFA_def_cfa_offset: {
+ auto Offset = Inst.getOperandAsSigned(CFIP, 0);
----------------
This and the next case are almost identical, except for a single string. I wonder if we could factor them out int a helper function or similar?
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:765
+
+ case dwarf::DW_CFA_def_cfa: {
+ auto RegNum = Inst.getOperandAsUnsigned(CFIP, 0);
----------------
These next two are identical again. Fold them together?
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:977-978
+ else
+ OS << " error: decoding the CIE opcodes into rows failed: "
+ << toString(RowsOrErr.takeError()) << "\n";
+ OS << "\n";
----------------
This sounds like it should be reported through an error handler, rather than printed into the main stream?
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:1003
+ else
+ OS << " error: decoding the CIE opcodes into rows failed: "
+ << toString(RowsOrErr.takeError()) << "\n";
----------------
Same comment as above.
================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:207
+ if (Cie == nullptr)
+ return createStringError(errc::illegal_byte_sequence,
+ "unable to get CIE for FDE at offset 0x%" PRIx64,
----------------
MaskRay wrote:
> The description of illegal_byte_sequence (EILSEQ) is: Invalid or incomplete multibyte or wide character. It is about an encoding problem. Probably just use errc::invalid_argument
@MaskRay, I thought that too, but I've been unable to find where the description comes from. Could you point me at it, please?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89845/new/
https://reviews.llvm.org/D89845
More information about the llvm-commits
mailing list