| Message ID | 1243881445-30069-2-git-send-email-naesten@gmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Mon, Jun 1, 2009 at 11:37 AM, Samuel Bronson <naesten@gmail.com> wrote: > Adjust the associated fields to match. While we're here, change > some nearby bitfields from "char" to "int" so GDB doesn't bother > showing character literals for them. > +enum modifier { > + MOD_AUTO = 1 << 0, The patch does not apply to my tree. The modifier is definitely not an enum because we apply bit operation to it. Changing bitfield from "char" to "int" has risk of making the symbol struct bigger. We don't want that because symbol is a very common structure for sparse. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 2, 2009 at 4:44 PM, Christopher Li <sparse@chrisli.org> wrote: > On Mon, Jun 1, 2009 at 11:37 AM, Samuel Bronson <naesten@gmail.com> wrote: >> Adjust the associated fields to match. While we're here, change >> some nearby bitfields from "char" to "int" so GDB doesn't bother >> showing character literals for them. > >> +enum modifier { >> + Â Â Â MOD_AUTO Â Â Â Â = 1 << 0, > > The patch does not apply to my tree. The modifier is definitely not > an enum because we apply bit operation to it. Hmm, which tree is yours? I pulled from git://git.kernel.org/pub/scm/devel/sparse/sparse.git As for the bit operations, those are perfectly legal for enum values, and there are two similar enum types defined in this very header: enum namespace { NS_NONE = 0, NS_MACRO = 1, NS_TYPEDEF = 2, NS_STRUCT = 4, // Also used for unions and enums. NS_LABEL = 8, NS_SYMBOL = 16, NS_ITERATOR = 32, NS_PREPROCESSOR = 64, NS_UNDEF = 128, NS_KEYWORD = 256, }; enum keyword { KW_SPECIFIER = 1 << 0, KW_MODIFIER = 1 << 1, KW_QUALIFIER = 1 << 2, KW_ATTRIBUTE = 1 << 3, KW_TYPEOF = 1 << 4, KW_STATEMENT = 1 << 5, KW_ASM = 1 << 6, KW_MODE = 1 << 7, }; > Changing bitfield from "char" to "int" has risk of making the symbol > struct bigger. We don't want that because symbol is a very common > structure for sparse. When could this actually happen? Here (on i386), pahole says this: struct symbol { enum type type:8; /* 0:24 4 */ enum namespace namespace:9; /* 0:15 4 */ unsigned int used:1; /* 0:14 4 */ enum sym_attr attr:2; /* 0:12 4 */ unsigned int enum_member:1; /* 0:11 4 */ unsigned int bound:1; /* 0:10 4 */ /* XXX 10 bits hole, try to pack */ struct position pos; /* 4 8 */ [...] union { struct basic_block * bb_target; /* 4 */ void * aux; /* 4 */ struct { char kind; /* 116 1 */ /* Bitfield combined with previous fields */ unsigned int visited:1; /* 116:23 4 */ }; /* 4 */ }; /* 116 4 */ If you run c2xml on the pahole output, you'll get: <symbol type="struct" id="_9" ident="position" file="-" start-line="10" start-col="8" end-line="21" end-col="2" bit-si ze="64" alignment="4" offset="0"> in the output, indicating that the 10-bit hole near the beginning is not due to the use of "unsigned int" instead of "unsigned char" for the bitfields before the hole, but due to the alignment required for the "struct position" following the hole. The "visited" bitfield doesn't cause a problem either, because it is packed into the same word as the "kind" field, and the other members of that union both take a word anyway. Looking it up in K&R 2nd edition, I see that the treatment of bitfields is almost entirely implementation-defined, so I unfortunately can't really make any general statements about this -- does anyone (perhaps someone on another architecture) have something to add? -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 2, 2009 at 8:43 PM, Samuel Bronson <naesten@gmail.com> wrote: > Hmm, which tree is yours? I pulled from > git://git.kernel.org/pub/scm/devel/sparse/sparse.git I have a development tree for sparse at: http://git.kernel.org/?p=devel/sparse/chrisl/sparse.git;a=summary > As for the bit operations, those are perfectly legal for enum values, > and there are two similar enum types defined in this very header: But they are simple assign, they don't have the following + MOD_NONLOCAL = (MOD_EXTERN | MOD_TOPLEVEL), + MOD_STORAGE = (MOD_AUTO | MOD_REGISTER | MOD_STATIC | MOD_EXTERN | MOD_INLINE | MOD_TOPLEVEL | MOD_FORCE), + MOD_SIGNEDNESS = (MOD_SIGNED | MOD_UNSIGNED | MOD_EXPLICITLY_SIGNED), + MOD_SPECIFIER = (MOD_CHAR | MOD_SHORT | MOD_LONG | MOD_LONGLONG | MOD_SIGNEDNESS), + MOD_SIZE = (MOD_CHAR | MOD_SHORT | MOD_LONG | MOD_LONGLONG), + MOD_IGNORE = (MOD_TOPLEVEL | MOD_STORAGE | MOD_ADDRESSABLE | + MOD_ASSIGNED | MOD_USERTYPE | MOD_ACCESSED | MOD_EXPLICITLY_SIGNED), + MOD_PTRINHERIT = (MOD_VOLATILE | MOD_CONST | MOD_NODEREF | MOD_STORAGE) Same can apply to pretty much all constant defines. It doesn't mean that we should replace all constant defines into enum. There is no actual benefit for people who runs sparse. >> Changing bitfield from "char" to "int" has risk of making the symbol >> struct bigger. We don't want that because symbol is a very common >> structure for sparse. > > When could this actually happen? Here (on i386), pahole says this: You answer yourself, it is implementation defined. Most of the people just run sparse. They don't benefit from your change. Personally I think how gdb print them is not a big deal. No worth to risk it. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/symbol.h b/symbol.h index c4d7f28..7c396a0 100644 --- a/symbol.h +++ b/symbol.h @@ -69,6 +69,59 @@ enum keyword { KW_MODE = 1 << 7, }; +enum sym_attr { + SYM_ATTR_WEAK = 0, + SYM_ATTR_NORMAL = 1, + SYM_ATTR_STRONG = 2, +}; + +/* Modifiers */ +enum modifier { + MOD_AUTO = 1 << 0, + MOD_REGISTER = 1 << 1, + MOD_STATIC = 1 << 2, + MOD_EXTERN = 1 << 3, + + MOD_CONST = 1 << 4, + MOD_VOLATILE = 1 << 5, + MOD_SIGNED = 1 << 6, + MOD_UNSIGNED = 1 << 7, + + MOD_CHAR = 1 << 8, + MOD_SHORT = 1 << 9, + MOD_LONG = 1 << 10, + MOD_LONGLONG = 1 << 11, + + MOD_TYPEDEF = 1 << 12, + + MOD_INLINE = 1 << 18, + MOD_ADDRESSABLE = 1 << 19, + + MOD_NOCAST = 1 << 20, + MOD_NODEREF = 1 << 21, + MOD_ACCESSED = 1 << 22, + MOD_TOPLEVEL = 1 << 23, // scoping.. + + MOD_LABEL = 1 << 24, + MOD_ASSIGNED = 1 << 25, + MOD_TYPE = 1 << 26, + MOD_SAFE = 1 << 27, // non-null/non-trapping pointer + + MOD_USERTYPE = 1 << 28, + MOD_FORCE = 1 << 29, + MOD_EXPLICITLY_SIGNED = 1 << 30, + MOD_BITWISE = 1 << 31, + + MOD_NONLOCAL = (MOD_EXTERN | MOD_TOPLEVEL), + MOD_STORAGE = (MOD_AUTO | MOD_REGISTER | MOD_STATIC | MOD_EXTERN | MOD_INLINE | MOD_TOPLEVEL | MOD_FORCE), + MOD_SIGNEDNESS = (MOD_SIGNED | MOD_UNSIGNED | MOD_EXPLICITLY_SIGNED), + MOD_SPECIFIER = (MOD_CHAR | MOD_SHORT | MOD_LONG | MOD_LONGLONG | MOD_SIGNEDNESS), + MOD_SIZE = (MOD_CHAR | MOD_SHORT | MOD_LONG | MOD_LONGLONG), + MOD_IGNORE = (MOD_TOPLEVEL | MOD_STORAGE | MOD_ADDRESSABLE | + MOD_ASSIGNED | MOD_USERTYPE | MOD_ACCESSED | MOD_EXPLICITLY_SIGNED), + MOD_PTRINHERIT = (MOD_VOLATILE | MOD_CONST | MOD_NODEREF | MOD_STORAGE), +}; + struct context { struct expression *context; unsigned int in, out, out_false; @@ -80,7 +133,7 @@ extern struct context *alloc_context(void); DECLARE_PTR_LIST(context_list, struct context); struct ctype { - unsigned long modifiers; + enum modifier modifiers; unsigned long alignment; struct context_list *contexts; unsigned int as; @@ -103,14 +156,13 @@ struct symbol_op { extern int expand_safe_p(struct expression *expr, int cost); extern int expand_constant_p(struct expression *expr, int cost); -#define SYM_ATTR_WEAK 0 -#define SYM_ATTR_NORMAL 1 -#define SYM_ATTR_STRONG 2 - struct symbol { enum type type:8; enum namespace namespace:9; - unsigned char used:1, attr:2, enum_member:1, bound:1; + unsigned int used:1; + enum sym_attr attr:2; + unsigned int enum_member:1; + unsigned int bound:1; struct position pos; /* Where this symbol was declared */ struct position endpos; /* Where this symbol ends*/ struct ident *ident; /* What identifier this symbol is associated with */ @@ -162,58 +214,12 @@ struct symbol { void *aux; /* Auxiliary info, e.g. backend information */ struct { /* sparse ctags */ char kind; - unsigned char visited:1; + unsigned int visited:1; }; }; pseudo_t pseudo; }; -/* Modifiers */ -#define MOD_AUTO 0x0001 -#define MOD_REGISTER 0x0002 -#define MOD_STATIC 0x0004 -#define MOD_EXTERN 0x0008 - -#define MOD_CONST 0x0010 -#define MOD_VOLATILE 0x0020 -#define MOD_SIGNED 0x0040 -#define MOD_UNSIGNED 0x0080 - -#define MOD_CHAR 0x0100 -#define MOD_SHORT 0x0200 -#define MOD_LONG 0x0400 -#define MOD_LONGLONG 0x0800 - -#define MOD_TYPEDEF 0x1000 - -#define MOD_INLINE 0x40000 -#define MOD_ADDRESSABLE 0x80000 - -#define MOD_NOCAST 0x100000 -#define MOD_NODEREF 0x200000 -#define MOD_ACCESSED 0x400000 -#define MOD_TOPLEVEL 0x800000 // scoping.. - -#define MOD_LABEL 0x1000000 -#define MOD_ASSIGNED 0x2000000 -#define MOD_TYPE 0x4000000 -#define MOD_SAFE 0x8000000 // non-null/non-trapping pointer - -#define MOD_USERTYPE 0x10000000 -#define MOD_FORCE 0x20000000 -#define MOD_EXPLICITLY_SIGNED 0x40000000 -#define MOD_BITWISE 0x80000000 - -#define MOD_NONLOCAL (MOD_EXTERN | MOD_TOPLEVEL) -#define MOD_STORAGE (MOD_AUTO | MOD_REGISTER | MOD_STATIC | MOD_EXTERN | MOD_INLINE | MOD_TOPLEVEL | MOD_FORCE) -#define MOD_SIGNEDNESS (MOD_SIGNED | MOD_UNSIGNED | MOD_EXPLICITLY_SIGNED) -#define MOD_SPECIFIER (MOD_CHAR | MOD_SHORT | MOD_LONG | MOD_LONGLONG | MOD_SIGNEDNESS) -#define MOD_SIZE (MOD_CHAR | MOD_SHORT | MOD_LONG | MOD_LONGLONG) -#define MOD_IGNORE (MOD_TOPLEVEL | MOD_STORAGE | MOD_ADDRESSABLE | \ - MOD_ASSIGNED | MOD_USERTYPE | MOD_ACCESSED | MOD_EXPLICITLY_SIGNED) -#define MOD_PTRINHERIT (MOD_VOLATILE | MOD_CONST | MOD_NODEREF | MOD_STORAGE) - - /* Current parsing/evaluation function */ extern struct symbol *current_fn;
Adjust the associated fields to match. While we're here, change some nearby bitfields from "char" to "int" so GDB doesn't bother showing character literals for them. This should allow the gdbhelpers script to work with more versions of GDB and/or build configurations. Signed-off-by: Samuel Bronson <naesten@gmail.com> --- symbol.h | 112 ++++++++++++++++++++++++++++++++----------------------------- 1 files changed, 59 insertions(+), 53 deletions(-)