<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Nov 10, 2016 at 11:53 AM Greg Clayton <<a href="mailto:clayborg@gmail.com">clayborg@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg added a comment.<br class="gmail_msg">
<br class="gmail_msg">
Will update according to comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:57<br class="gmail_msg">
<br class="gmail_msg">
+Optional<uint8_t> DWARFFormValue::getFixedByteSize(dwarf::Form form,<br class="gmail_msg">
+ const DWARFUnit *U) {<br class="gmail_msg">
----------------<br class="gmail_msg">
aprantl wrote:<br class="gmail_msg">
> Would it make sense / be possible to encode this tabular data (at least for the majority of values) in the macros in Dwarf.def?<br class="gmail_msg">
Not sure how you would do this since many don't have a byte size. Zero is a valid DW_FORM size, so you can't use zero for items that don't have byte sizes. I wouldn't want to have a signed integer as the size either where -1 would indicate that there is no size.</blockquote><div><br>Why would this (signed values with -1 as a flag value, or unsigned with MAX_INT as the flag) be particularly problematic?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> So since there is no easy way to encode an optional value with no value into the table, I would vote against this. </blockquote><div><br></div><div>'None' would be the empty Optional value, which could be in the table o' macros.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Agains, this is the only function that needs to be updated.</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:364<br class="gmail_msg">
<br class="gmail_msg">
-bool<br class="gmail_msg">
-DWARFFormValue::skipValue(DataExtractor debug_info_data, uint32_t* offset_ptr,<br class="gmail_msg">
- const DWARFUnit *U) const {<br class="gmail_msg">
- return DWARFFormValue::skipValue(Form, debug_info_data, offset_ptr, U);<br class="gmail_msg">
-}<br class="gmail_msg">
-<br class="gmail_msg">
-bool<br class="gmail_msg">
-DWARFFormValue::skipValue(dwarf::Form form, DataExtractor debug_info_data,<br class="gmail_msg">
- uint32_t *offset_ptr, const DWARFUnit *cu) {<br class="gmail_msg">
- return skipValue(form, debug_info_data, offset_ptr, cu->getVersion(),<br class="gmail_msg">
- cu->getAddressByteSize());<br class="gmail_msg">
-}<br class="gmail_msg">
-bool<br class="gmail_msg">
-DWARFFormValue::skipValue(dwarf::Form form, DataExtractor debug_info_data,<br class="gmail_msg">
- uint32_t *offset_ptr, uint16_t Version,<br class="gmail_msg">
- uint8_t AddrSize) {<br class="gmail_msg">
+bool skipVariableLengthValue(dwarf::Form form,<br class="gmail_msg">
+ const DataExtractor &debug_info_data,<br class="gmail_msg">
----------------<br class="gmail_msg">
aprantl wrote:<br class="gmail_msg">
> I know it was like this in the original source, but while you are editing it, we could use the opportunity fix the variable names to start with uppercase (<a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly" rel="noreferrer" class="gmail_msg" target="_blank">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a>). Up to you.<br class="gmail_msg">
I desire to keep this patch small so the extra diffs don't cloud our ability to see changes. I will do a follow up patch where I do nothing but case changing.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:424<br class="gmail_msg">
+ } else {<br class="gmail_msg">
+ assert(!"only variable length Form can be passed to this function");<br class="gmail_msg">
+ }<br class="gmail_msg">
----------------<br class="gmail_msg">
aprantl wrote:<br class="gmail_msg">
> Should this really be an assertion, in the sense of that it indicates a programming error in the caller? Or can this situation be triggered by malformed DWARF input and should we return an Error instead?<br class="gmail_msg">
This function is internal to this file and is designed to only be called for variable length forms. The callers of this function both do:<br class="gmail_msg">
<br class="gmail_msg">
```<br class="gmail_msg">
if (Optional<uint8_t> FixedByteSize = getFixedByteSize(form, U)) {<br class="gmail_msg">
*offset_ptr += *FixedByteSize;<br class="gmail_msg">
return true;<br class="gmail_msg">
}<br class="gmail_msg">
return skipVariableLengthValue(...);<br class="gmail_msg">
```<br class="gmail_msg">
<br class="gmail_msg">
I turned this into an llvm_unreachable().<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D26514" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D26514</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>