[llvm] r191025 - DebugInfo: Simplify gnu_pubnames index computation.

David Blaikie dblaikie at gmail.com
Thu Sep 19 13:30:21 PDT 2013


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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130919/c7e3a90b/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gnu_pubnames_constants.diff
Type: application/octet-stream
Size: 6948 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130919/c7e3a90b/attachment.obj>


More information about the llvm-commits mailing list