<div dir="ltr">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.<div>
<br></div><div>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.<br>
<br>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)))</div>
<div><div><br></div><div>*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.</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Sep 19, 2013 at 11:39 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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: <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>
--- 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 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 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: <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>
--- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)<br>
+++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Thu Sep 19 13:39:59 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>
- (CURRENT) |= (((VALUE) & dwarf::GDB_INDEX_SYMBOL_KIND_MASK) \<br>
- << dwarf::GDB_INDEX_SYMBOL_KIND_SHIFT); \<br>
- }<br>
-<br>
-#define UPDATE_STATIC(CURRENT, IS_STATIC) \<br>
- { \<br>
- (CURRENT) |= (((IS_STATIC) & dwarf::GDB_INDEX_SYMBOL_STATIC_MASK) \<br>
- << dwarf::GDB_INDEX_SYMBOL_STATIC_SHIFT); \<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 | 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>
</blockquote></div><br></div></div>