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