<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Feb 21, 2013 at 4:50 PM, Eli Bendersky <span dir="ltr"><<a href="mailto:eliben@google.com" target="_blank">eliben@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: eliben<br>
Date: Thu Feb 21 18:50:48 2013<br>
New Revision: 175847<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=175847&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=175847&view=rev</a><br>
Log:<br>
Code cleanup: pass Offset by pointer to parseInstruction to more explicitly<br>
convey that it's a INOUT argument.<br></blockquote><div><br></div><div style>This isn't really a convention we have in the LLVM code base & it's not one I'd personally prefer we adopt (though my opinion isn't the only one, of course). Mostly because I always wonder whether such a parameter can be null (from both the caller & the callee) and what it means if it is.<br>
<br>Is there some other way we could tweak such an API to communicate this? Though honestly I've seen enough parse/cursor APIs that this doesn't seem /that/ unusual/confusing. Was there a particularly confusing call site you saw?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, if parsing of entry instructions fails, don't push the entry.<br>
<br>
<br>
Modified:<br>
llvm/trunk/lib/DebugInfo/DWARFDebugFrame.cpp<br>
<br>
Modified: llvm/trunk/lib/DebugInfo/DWARFDebugFrame.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/DWARFDebugFrame.cpp?rev=175847&r1=175846&r2=175847&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/DWARFDebugFrame.cpp?rev=175847&r1=175846&r2=175847&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/DebugInfo/DWARFDebugFrame.cpp (original)<br>
+++ llvm/trunk/lib/DebugInfo/DWARFDebugFrame.cpp Thu Feb 21 18:50:48 2013<br>
@@ -36,11 +36,10 @@ public:<br>
virtual uint64_t getOffset() const { return Offset; }<br>
<br>
/// \brief Parse and store a sequence of CFI instructions from our data<br>
- /// stream, starting at Offset and ending at EndOffset. If everything<br>
- /// goes well, Offset should be equal to EndOffset when this method<br>
+ /// stream, starting at *Offset and ending at EndOffset. If everything<br>
+ /// goes well, *Offset should be equal to EndOffset when this method<br>
/// returns. Otherwise, an error occurred.<br>
- /// TODO: Improve error reporting...<br>
- virtual void parseInstructions(uint32_t &Offset, uint32_t EndOffset);<br>
+ virtual void parseInstructions(uint32_t *Offset, uint32_t EndOffset);<br>
<br>
/// \brief Dump the entry header to the given output stream.<br>
virtual void dumpHeader(raw_ostream &OS) const = 0;<br>
@@ -99,9 +98,9 @@ const uint8_t DWARF_CFI_PRIMARY_OPCODE_M<br>
const uint8_t DWARF_CFI_PRIMARY_OPERAND_MASK = 0x3f;<br>
<br>
<br>
-void FrameEntry::parseInstructions(uint32_t &Offset, uint32_t EndOffset) {<br>
- while (Offset < EndOffset) {<br>
- uint8_t Opcode = Data.getU8(&Offset);<br>
+void FrameEntry::parseInstructions(uint32_t *Offset, uint32_t EndOffset) {<br>
+ while (*Offset < EndOffset) {<br>
+ uint8_t Opcode = Data.getU8(Offset);<br>
// Some instructions have a primary opcode encoded in the top bits.<br>
uint8_t Primary = Opcode & DWARF_CFI_PRIMARY_OPCODE_MASK;<br>
<br>
@@ -116,7 +115,7 @@ void FrameEntry::parseInstructions(uint3<br>
addInstruction(Primary, Op1);<br>
break;<br>
case DW_CFA_offset:<br>
- addInstruction(Primary, Op1, Data.getULEB128(&Offset));<br>
+ addInstruction(Primary, Op1, Data.getULEB128(Offset));<br>
break;<br>
}<br>
} else {<br>
@@ -131,19 +130,19 @@ void FrameEntry::parseInstructions(uint3<br>
break;<br>
case DW_CFA_set_loc:<br>
// Operands: Address<br>
- addInstruction(Opcode, Data.getAddress(&Offset));<br>
+ addInstruction(Opcode, Data.getAddress(Offset));<br>
break;<br>
case DW_CFA_advance_loc1:<br>
// Operands: 1-byte delta<br>
- addInstruction(Opcode, Data.getU8(&Offset));<br>
+ addInstruction(Opcode, Data.getU8(Offset));<br>
break;<br>
case DW_CFA_advance_loc2:<br>
// Operands: 2-byte delta<br>
- addInstruction(Opcode, Data.getU16(&Offset));<br>
+ addInstruction(Opcode, Data.getU16(Offset));<br>
break;<br>
case DW_CFA_advance_loc4:<br>
// Operands: 4-byte delta<br>
- addInstruction(Opcode, Data.getU32(&Offset));<br>
+ addInstruction(Opcode, Data.getU32(Offset));<br>
break;<br>
case DW_CFA_restore_extended:<br>
case DW_CFA_undefined:<br>
@@ -151,26 +150,26 @@ void FrameEntry::parseInstructions(uint3<br>
case DW_CFA_def_cfa_register:<br>
case DW_CFA_def_cfa_offset:<br>
// Operands: ULEB128<br>
- addInstruction(Opcode, Data.getULEB128(&Offset));<br>
+ addInstruction(Opcode, Data.getULEB128(Offset));<br>
break;<br>
case DW_CFA_def_cfa_offset_sf:<br>
// Operands: SLEB128<br>
- addInstruction(Opcode, Data.getSLEB128(&Offset));<br>
+ addInstruction(Opcode, Data.getSLEB128(Offset));<br>
break;<br>
case DW_CFA_offset_extended:<br>
case DW_CFA_register:<br>
case DW_CFA_def_cfa:<br>
case DW_CFA_val_offset:<br>
// Operands: ULEB128, ULEB128<br>
- addInstruction(Opcode, Data.getULEB128(&Offset),<br>
- Data.getULEB128(&Offset));<br>
+ addInstruction(Opcode, Data.getULEB128(Offset),<br>
+ Data.getULEB128(Offset));<br>
break;<br>
case DW_CFA_offset_extended_sf:<br>
case DW_CFA_def_cfa_sf:<br>
case DW_CFA_val_offset_sf:<br>
// Operands: ULEB128, SLEB128<br>
- addInstruction(Opcode, Data.getULEB128(&Offset),<br>
- Data.getSLEB128(&Offset));<br>
+ addInstruction(Opcode, Data.getULEB128(Offset),<br>
+ Data.getSLEB128(Offset));<br>
break;<br>
case DW_CFA_def_cfa_expression:<br>
case DW_CFA_expression:<br>
@@ -337,37 +336,42 @@ void DWARFDebugFrame::parse(DataExtracto<br>
Id = Data.getUnsigned(&Offset, IsDWARF64 ? 8 : 4);<br>
bool IsCIE = ((IsDWARF64 && Id == DW64_CIE_ID) || Id == DW_CIE_ID);<br>
<br>
+ FrameEntry *Entry = 0;<br>
if (IsCIE) {<br>
// Note: this is specifically DWARFv3 CIE header structure. It was<br>
- // changed in DWARFv4.<br>
+ // changed in DWARFv4. We currently don't support reading DWARFv4<br>
+ // here because LLVM itself does not emit it (and LLDB doesn't<br>
+ // support it either).<br>
uint8_t Version = Data.getU8(&Offset);<br>
const char *Augmentation = Data.getCStr(&Offset);<br>
uint64_t CodeAlignmentFactor = Data.getULEB128(&Offset);<br>
int64_t DataAlignmentFactor = Data.getSLEB128(&Offset);<br>
uint64_t ReturnAddressRegister = Data.getULEB128(&Offset);<br>
<br>
- CIE *NewCIE = new CIE(Data, StartOffset, Length, Version,<br>
- StringRef(Augmentation), CodeAlignmentFactor,<br>
- DataAlignmentFactor, ReturnAddressRegister);<br>
- Entries.push_back(NewCIE);<br>
+ Entry = new CIE(Data, StartOffset, Length, Version,<br>
+ StringRef(Augmentation), CodeAlignmentFactor,<br>
+ DataAlignmentFactor, ReturnAddressRegister);<br>
} else {<br>
// FDE<br>
uint64_t CIEPointer = Id;<br>
uint64_t InitialLocation = Data.getAddress(&Offset);<br>
uint64_t AddressRange = Data.getAddress(&Offset);<br>
<br>
- FDE *NewFDE = new FDE(Data, StartOffset, Length, CIEPointer,<br>
- InitialLocation, AddressRange);<br>
- Entries.push_back(NewFDE);<br>
+ Entry = new FDE(Data, StartOffset, Length, CIEPointer,<br>
+ InitialLocation, AddressRange);<br>
}<br>
<br>
- Entries.back()->parseInstructions(Offset, EndStructureOffset);<br>
+ assert(Entry && "Expected Entry to be populated with CIE or FDE");<br>
+ Entry->parseInstructions(&Offset, EndStructureOffset);<br>
<br>
- if (Offset != EndStructureOffset) {<br>
+ if (Offset == EndStructureOffset) {<br>
+ // Entry instrucitons parsed successfully.<br>
+ Entries.push_back(Entry);<br>
+ } else {<br>
std::string Str;<br>
raw_string_ostream OS(Str);<br>
OS << format("Parsing entry instructions at %lx failed",<br>
- Entries.back()->getOffset());<br>
+ Entry->getOffset());<br>
report_fatal_error(Str);<br>
}<br>
}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>