<div dir="ltr">Thanks. r191035.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Sep 19, 2013 at 1:33 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Sold. Thanks.<br>
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Thu, Sep 19, 2013 at 1:30 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> Eric made a good point offline that these constants aren't only used in this<br>
> particular record layout. As mentioned in the comments, in the full<br>
> gdb_index these 2 fields are all shifted up 24 bits. Rather than having to<br>
> take the byte, shift it down, then split it & shift it some more to extract<br>
> the two values based on the shift/mask constants here, it might* be nicer to<br>
> keep the constants unshifted for more general use, then shift them the<br>
> appropriate amount for the current record being dealt with.<br>
><br>
> With that in mind, Eric, here's another take on this that avoids the curious<br>
> case of "shifting up & down" while preserving the unshifted constants and<br>
> wrapping things up/making them more type safe, perhaps.<br>
><br>
> Does this make more sense? (minor tidbits: I left the bit->record ctor in,<br>
> though it's unused. I'll be using it in the dumping support shortly, but<br>
> can/may pull it out to go in with the usage instead. I'll also probably add<br>
> a "std::string ToString()" function to print this out for annotation<br>
> comments and dumping support. (that would mean a fixed layout for the static<br>
> and kind values - whereas two general purpose toString functions over the<br>
> two enums would be more flexible for clients but might involve some<br>
> repetition (I suppose I could provide a utility function to do the two<br>
> together in a fixed format for the annotation comments while still leaving<br>
> the two general purpose functions as well)))<br>
><br>
> *actually treating these two fields as a single nested record, shifting and<br>
> masking that record out of the larger record doesn't seem so bad, in fact<br>
> it'd depend on the file reading APIs - reading the byte would be easy, but<br>
> reading the remaining 3 bytes for the CU offset in the full gdb_index record<br>
> would be a bit awkward, so it's likely it'll be read as a single 4 byte<br>
> value then shifting out the high byte to retrieve the kind and static-ness.<br>
> I suppose even then you could still mask out the high byte and process that<br>
> using the APIs as committed here in r191025, though I rather like wrapping<br>
> this up with a struct, some specific enums, etc.<br>
><br>
><br>
> On Thu, Sep 19, 2013 at 11:39 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> Author: dblaikie<br>
>> Date: Thu Sep 19 13:39:59 2013<br>
>> New Revision: 191025<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=191025&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=191025&view=rev</a><br>
>> Log:<br>
>> DebugInfo: Simplify gnu_pubnames index computation.<br>
>><br>
>> Names open to bikeshedding. Could switch back to the constants being<br>
>> unshifted, but this way seems a bit easier to work with.<br>
>><br>
>> Modified:<br>
>> llvm/trunk/include/llvm/Support/Dwarf.h<br>
>> llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp<br>
>><br>
>> Modified: llvm/trunk/include/llvm/Support/Dwarf.h<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Dwarf.h?rev=191025&r1=191024&r2=191025&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Dwarf.h?rev=191025&r1=191024&r2=191025&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/include/llvm/Support/Dwarf.h (original)<br>
>> +++ llvm/trunk/include/llvm/Support/Dwarf.h Thu Sep 19 13:39:59 2013<br>
>> @@ -791,37 +791,34 @@ const char *AtomTypeString(unsigned Atom<br>
>><br>
>> // Constants for the GNU pubnames/pubtypes extensions supporting gdb<br>
>> index.<br>
>> enum GDBIndex {<br>
>> - // The full index looks like this for each symbol:<br>
>> + // The gnu_pub* index value looks like:<br>
>> //<br>
>> - // 0-23 CU index<br>
>> - // 24-27 reserved<br>
>> - // 28-30 symbol kind<br>
>> - // 31 0 == global, 1 == static<br>
>> - //<br>
>> - // where each entry refers to the CU and some attributes about the<br>
>> symbol.<br>
>> + // 0-3 reserved<br>
>> + // 4-6 symbol kind<br>
>> + // 7 0 == global, 1 == static<br>
>><br>
>> // Attributes kinds for the index.<br>
>> + GDB_INDEX_SYMBOL_KIND_OFFSET = 4,<br>
>> + GDB_INDEX_SYMBOL_KIND_MASK = 7 << GDB_INDEX_SYMBOL_KIND_OFFSET,<br>
>><br>
>> // Special value to indicate no attributes are present.<br>
>> GDB_INDEX_SYMBOL_KIND_NONE = 0,<br>
>> - GDB_INDEX_SYMBOL_KIND_TYPE = 1,<br>
>> - GDB_INDEX_SYMBOL_KIND_VARIABLE = 2,<br>
>> - GDB_INDEX_SYMBOL_KIND_FUNCTION = 3,<br>
>> - GDB_INDEX_SYMBOL_KIND_OTHER = 4,<br>
>> - // 3 unused bits.<br>
>> - GDB_INDEX_SYMBOL_KIND_UNUSED5 = 5,<br>
>> - GDB_INDEX_SYMBOL_KIND_UNUSED6 = 6,<br>
>> - GDB_INDEX_SYMBOL_KIND_UNUSED7 = 7,<br>
>> + GDB_INDEX_SYMBOL_KIND_TYPE = 1 << GDB_INDEX_SYMBOL_KIND_OFFSET,<br>
>> + GDB_INDEX_SYMBOL_KIND_VARIABLE = 2 << GDB_INDEX_SYMBOL_KIND_OFFSET,<br>
>> + GDB_INDEX_SYMBOL_KIND_FUNCTION = 3 << GDB_INDEX_SYMBOL_KIND_OFFSET,<br>
>> + GDB_INDEX_SYMBOL_KIND_OTHER = 4 << GDB_INDEX_SYMBOL_KIND_OFFSET,<br>
>> + // 3 unused values.<br>
>> + GDB_INDEX_SYMBOL_KIND_UNUSED5 = 5 << GDB_INDEX_SYMBOL_KIND_OFFSET,<br>
>> + GDB_INDEX_SYMBOL_KIND_UNUSED6 = 6 << GDB_INDEX_SYMBOL_KIND_OFFSET,<br>
>> + GDB_INDEX_SYMBOL_KIND_UNUSED7 = 7 << GDB_INDEX_SYMBOL_KIND_OFFSET,<br>
>><br>
>> // Index values are defined via the set of CUs that define the<br>
>> // symbol. For the pubnames/pubtypes extensions we need the<br>
>> // various shifts and masks.<br>
>> - GDB_INDEX_SYMBOL_STATIC_SHIFT = 31,<br>
>> - GDB_INDEX_SYMBOL_STATIC_MASK = 1,<br>
>> - GDB_INDEX_SYMBOL_KIND_SHIFT = 28,<br>
>> - GDB_INDEX_SYMBOL_KIND_MASK = 7,<br>
>> - GDB_INDEX_CU_BITSIZE = 24,<br>
>> - GDB_INDEX_CU_MASK = ((1 << GDB_INDEX_CU_BITSIZE) - 1)<br>
>> + GDB_INDEX_SYMBOL_STATIC_OFFSET = 7,<br>
>> + GDB_INDEX_SYMBOL_STATIC_MASK = 1 << GDB_INDEX_SYMBOL_STATIC_OFFSET,<br>
>> + GDB_INDEX_SYMBOL_STATIC = 1 << GDB_INDEX_SYMBOL_STATIC_OFFSET,<br>
>> + GDB_INDEX_SYMBOL_NON_STATIC = 0<br>
>> };<br>
>><br>
>> /// GDBIndexTypeString - Return the string for the specified index type.<br>
>><br>
>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=191025&r1=191024&r2=191025&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=191025&r1=191024&r2=191025&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)<br>
>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Thu Sep 19 13:39:59<br>
>> 2013<br>
>> @@ -2323,21 +2323,9 @@ void DwarfDebug::emitAccelTypes() {<br>
>><br>
>> /// computeIndexValue - Compute the gdb index value for the DIE and CU.<br>
>> static uint8_t computeIndexValue(CompileUnit *CU, DIE *Die) {<br>
>> -#define UPDATE_VALUE(CURRENT, VALUE)<br>
>> \<br>
>> - {<br>
>> \<br>
>> - (CURRENT) |= (((VALUE) & dwarf::GDB_INDEX_SYMBOL_KIND_MASK)<br>
>> \<br>
>> - << dwarf::GDB_INDEX_SYMBOL_KIND_SHIFT);<br>
>> \<br>
>> - }<br>
>> -<br>
>> -#define UPDATE_STATIC(CURRENT, IS_STATIC)<br>
>> \<br>
>> - {<br>
>> \<br>
>> - (CURRENT) |= (((IS_STATIC) & dwarf::GDB_INDEX_SYMBOL_STATIC_MASK)<br>
>> \<br>
>> - << dwarf::GDB_INDEX_SYMBOL_STATIC_SHIFT);<br>
>> \<br>
>> - }<br>
>> -<br>
>> - // Compute the Attributes for the Die.<br>
>> - uint32_t Value = dwarf::GDB_INDEX_SYMBOL_KIND_NONE;<br>
>> - bool External = Die->findAttribute(dwarf::DW_AT_external);<br>
>> + uint8_t IsStatic = Die->findAttribute(dwarf::DW_AT_external)<br>
>> + ? dwarf::GDB_INDEX_SYMBOL_NON_STATIC<br>
>> + : dwarf::GDB_INDEX_SYMBOL_STATIC;<br>
>><br>
>> switch (Die->getTag()) {<br>
>> case dwarf::DW_TAG_class_type:<br>
>> @@ -2347,33 +2335,20 @@ static uint8_t computeIndexValue(Compile<br>
>> case dwarf::DW_TAG_typedef:<br>
>> case dwarf::DW_TAG_base_type:<br>
>> case dwarf::DW_TAG_subrange_type:<br>
>> - UPDATE_VALUE(Value, dwarf::GDB_INDEX_SYMBOL_KIND_TYPE);<br>
>> - UPDATE_STATIC(Value, 1);<br>
>> - break;<br>
>> + return dwarf::GDB_INDEX_SYMBOL_KIND_TYPE |<br>
>> dwarf::GDB_INDEX_SYMBOL_STATIC;<br>
>> case dwarf::DW_TAG_namespace:<br>
>> - UPDATE_VALUE(Value, dwarf::GDB_INDEX_SYMBOL_KIND_TYPE);<br>
>> - break;<br>
>> + return dwarf::GDB_INDEX_SYMBOL_KIND_TYPE;<br>
>> case dwarf::DW_TAG_subprogram:<br>
>> - UPDATE_VALUE(Value, dwarf::GDB_INDEX_SYMBOL_KIND_FUNCTION);<br>
>> - UPDATE_STATIC(Value, !External);<br>
>> - break;<br>
>> + return dwarf::GDB_INDEX_SYMBOL_KIND_FUNCTION | IsStatic;<br>
>> case dwarf::DW_TAG_constant:<br>
>> case dwarf::DW_TAG_variable:<br>
>> - UPDATE_VALUE(Value, dwarf::GDB_INDEX_SYMBOL_KIND_VARIABLE);<br>
>> - UPDATE_STATIC(Value, !External);<br>
>> - break;<br>
>> + return dwarf::GDB_INDEX_SYMBOL_KIND_VARIABLE | IsStatic;<br>
>> case dwarf::DW_TAG_enumerator:<br>
>> - UPDATE_VALUE(Value, dwarf::GDB_INDEX_SYMBOL_KIND_VARIABLE);<br>
>> - UPDATE_STATIC(Value, 1);<br>
>> - break;<br>
>> + return dwarf::GDB_INDEX_SYMBOL_KIND_VARIABLE |<br>
>> + dwarf::GDB_INDEX_SYMBOL_STATIC;<br>
>> default:<br>
>> - break;<br>
>> + return dwarf::GDB_INDEX_SYMBOL_KIND_NONE;<br>
>> }<br>
>> - // We don't need to add the CU into the bitmask for two reasons:<br>
>> - // a) the pubnames/pubtypes sections are per-cu, and<br>
>> - // b) the linker wouldn't understand it anyhow.<br>
>> - // so go ahead and make it 1 byte by shifting it down.<br>
>> - return Value >> dwarf::GDB_INDEX_CU_BITSIZE;<br>
>> }<br>
>><br>
>> /// emitDebugPubNames - Emit visible names into a debug pubnames section.<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>
><br>
><br>
</div></div></blockquote></div><br></div>