[llvm] r191025 - DebugInfo: Simplify gnu_pubnames index computation.
David Blaikie
dblaikie at gmail.com
Thu Sep 19 13:43:21 PDT 2013
Thanks. r191035.
On Thu, Sep 19, 2013 at 1:33 PM, Eric Christopher <echristo at gmail.com>wrote:
> 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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130919/542e1ddc/attachment.html>
More information about the llvm-commits
mailing list