modifier_string() inconsistency with modifiers

modifier_string() inconsistency with modifiers

[Bernd Petrovitsch]

Hi all!

To dump/print the modifiers, I just found the
modifier_string() function (in show-parse.c).

To the best of my understanding
----  snip  ----
	const char *res,**ptr, *names[] = {
		"auto", "register", "static", "extern",
		"const", "volatile", "[signed]", "[unsigned]",
		"[char]", "[short]", "[long]", "[long long]",
		"[typdef]", "[structof]", "[unionof]", "[enum]",
		"[typeof]", "[attribute]", "inline", "[addressable]",
		"[nocast]", "[noderef]", "[accessed]", "[toplevel]",
		"[label]", "[assigned]", "[type]", "[safe]",
		"[usertype]", "[force]", "[explicitly-signed]",
		NULL
	};
----  snip  ----
lists printable strings for the modifiers. The modifiers are bits
defined in symbol.h.
----  snip  ----
/* 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_WEAK	0x2000

#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
----  snip  ----
Well, the equivalent for "MOD_WEAK" is "[structof]" - which seems wrong
to me.
And the parts for "inline" also doesn't fit AFAICS.

What did I actually miss?

	Bernd
-- 
mobile: +43 664 4416156              http://www.sysprog.at/
    Linux Software Development, Consulting and Services

Re: modifier_string() inconsistency with modifiers

[Christopher Li]

On Fri, Aug 20, 2010 at 3:33 AM, Bernd Petrovitsch <bernd@sysprog.at> wrote:
>
> Well, the equivalent for "MOD_WEAK" is "[structof]" - which seems wrong
> to me.
> And the parts for "inline" also doesn't fit AFAICS.
>
> What did I actually miss?

Nothing. We remove some bits from the modifier but forget to update
the modifier_string function.

Care for a patch?

Chris

Re: modifier_string() inconsistency with modifiers

[Bernd Petrovitsch]

On Fre, 2010-08-20 at 16:03 -0700, Christopher Li wrote:
> On Fri, Aug 20, 2010 at 3:33 AM, Bernd Petrovitsch <bernd@sysprog.at> wrote:
> >
> > Well, the equivalent for "MOD_WEAK" is "[structof]" - which seems wrong
> > to me.
> > And the parts for "inline" also doesn't fit AFAICS.
> >
> > What did I actually miss?
> 
> Nothing. We remove some bits from the modifier but forget to update
> the modifier_string function.
> 
> Care for a patch?

Not a big problem.
Hmm, is it prudent to use c99, e.g. for named initializers und array?

	Bernd
-- 
mobile: +43 664 4416156              http://www.sysprog.at/
    Linux Software Development, Consulting and Services

Re: modifier_string() inconsistency with modifiers

[Christopher Li]

On Sun, Aug 22, 2010 at 1:42 AM, Bernd Petrovitsch <bernd@sysprog.at> wrote:
> Not a big problem.
> Hmm, is it prudent to use c99, e.g. for named initializers und array?
>
Not at all. Sparse use c99 extensively.

Chris

[PATCH] Fixup and cleanup of the modifier_string() function (was Re: modifier_string() inconsistency with modifiers)

[Bernd Petrovitsch]

Fixup and cleanup of the modifier_string() function

This patch does:
- it fixes the modifier_string() function
- The array with the names is made "static".
- We use C99-indexed initializers. For this we add a set of
  #defines with the bit number.
- replace the open-coded string copy without length check
  with a snprintf()-based one.

Signed-off-by: Bernd Petrovitsch <bernd@sysprog.at>
---
 show-parse.c |   63 +++++++++++++++++++++++-----------
 symbol.h     |  106 +++++++++++++++++++++++++++++++++++++++-------------------
 2 files changed, 114 insertions(+), 55 deletions(-)

diff --git a/show-parse.c b/show-parse.c
index b771018..34227b4 100644
--- a/show-parse.c
+++ b/show-parse.c
@@ -94,31 +94,54 @@ void debug_symbol(struct symbol *sym)
  */
 const char *modifier_string(unsigned long mod)
 {
+	static const char *names[] = {
+		[MOD_AUTO_BIT]			= "auto",
+		[MOD_REGISTER_BIT]		= "register",
+		[MOD_STATIC_BIT]		= "static",
+		[MOD_EXTERN_BIT]		= "extern",
+
+		[MOD_CONST_BIT]			= "const",
+		[MOD_VOLATILE_BIT]		= "volatile",
+		[MOD_SIGNED_BIT]		= "[signed]",
+		[MOD_UNSIGNED_BIT]		= "[unsigned]",
+
+		[MOD_CHAR_BIT]			= "[char]",
+		[MOD_SHORT_BIT]			= "[short]",
+		[MOD_LONG_BIT]			= "[long]",
+		[MOD_LONGLONG_BIT]		= "[long long]",
+
+		[MOD_TYPEDEF_BIT]		= "[typedef]",
+		[MOD_WEAK_BIT]			= "[weak]",
+
+		[MOD_INLINE_BIT]		= "inline",
+		[MOD_ADDRESSABLE_BIT]		= "[addressable]",
+
+		[MOD_NOCAST_BIT]		= "[nocast]",
+		[MOD_NODEREF_BIT]		= "[noderef]",
+		[MOD_ACCESSED_BIT]		= "[accessed]",
+		[MOD_TOPLEVEL_BIT]		= "[toplevel]",
+
+		[MOD_LABEL_BIT]			= "[label]",
+		[MOD_ASSIGNED_BIT]		= "[assigned]",
+		[MOD_TYPE_BIT]			= "[type]",
+		[MOD_SAFE_BIT]			= "[safe]",
+
+		[MOD_USERTYPE_BIT]		= "[usertype]",
+		[MOD_FORCE_BIT]			= "[force]",
+		[MOD_EXPLICITLY_SIGNED_BIT]	= "[explicitly-signed]",
+		[MOD_BITWISE_BIT]		= "[bitwise]",
+        };
 	static char buffer[100];
-	char *p = buffer;
-	const char *res,**ptr, *names[] = {
-		"auto", "register", "static", "extern",
-		"const", "volatile", "[signed]", "[unsigned]",
-		"[char]", "[short]", "[long]", "[long long]",
-		"[typedef]", "[structof]", "[unionof]", "[enum]",
-		"[typeof]", "[attribute]", "inline", "[addressable]",
-		"[nocast]", "[noderef]", "[accessed]", "[toplevel]",
-		"[label]", "[assigned]", "[type]", "[safe]",
-		"[usertype]", "[force]", "[explicitly-signed]",
-		NULL
-	};
-	ptr = names;
-	while ((res = *ptr++) != NULL) {
+	int len = 0;
+        unsigned int i;
+
+	for (i = 0; i < sizeof(names)/sizeof(names[0]) && len < sizeof(buffer); i++) {
 		if (mod & 1) {
-			char c;
-			while ((c = *res++) != '\0')
-				*p++ = c;
-			*p++ = ' ';
+			len += snprintf(buffer+len, sizeof(buffer)-len, " %s", names[i]);
 		}
 		mod >>= 1;
 	}
-	*p = 0;
-	return buffer;
+	return buffer+1; /* hide the first space */
 }
 
 static void show_struct_member(struct symbol *sym)
diff --git a/symbol.h b/symbol.h
index b3fcccd..b66e2a1 100644
--- a/symbol.h
+++ b/symbol.h
@@ -169,41 +169,77 @@ struct symbol {
 };
 
 /* 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_WEAK	0x2000
-
-#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_AUTO_BIT			 0
+#define MOD_REGISTER_BIT		 1
+#define MOD_STATIC_BIT			 2
+#define MOD_EXTERN_BIT			 3
+
+#define MOD_CONST_BIT			 4
+#define MOD_VOLATILE_BIT		 5
+#define MOD_SIGNED_BIT			 6
+#define MOD_UNSIGNED_BIT		 7
+
+#define MOD_CHAR_BIT			 8
+#define MOD_SHORT_BIT			 9
+#define MOD_LONG_BIT			10
+#define MOD_LONGLONG_BIT		11
+
+#define MOD_TYPEDEF_BIT			12
+#define MOD_WEAK_BIT			13
+#define MOD_INLINE_BIT			14
+#define MOD_ADDRESSABLE_BIT		15
+
+#define MOD_NOCAST_BIT			16
+#define MOD_NODEREF_BIT			17
+#define MOD_ACCESSED_BIT		18
+#define MOD_TOPLEVEL_BIT		19		// scoping..
+
+#define MOD_LABEL_BIT			20
+#define MOD_ASSIGNED_BIT		21
+#define MOD_TYPE_BIT			22
+#define MOD_SAFE_BIT			23		// non-null/non-trapping pointer
+
+#define MOD_USERTYPE_BIT		24
+#define MOD_FORCE_BIT			25
+#define MOD_EXPLICITLY_SIGNED_BIT	26
+#define MOD_BITWISE_BIT			27
+
+/* use the following in the code - we need the above to initialize an array */
+#define MOD_AUTO		(1 << MOD_AUTO_BIT)
+#define MOD_REGISTER		(1 << MOD_REGISTER_BIT)
+#define MOD_STATIC		(1 << MOD_STATIC_BIT)
+#define MOD_EXTERN		(1 << MOD_EXTERN_BIT)
+
+#define MOD_CONST		(1 << MOD_CONST_BIT)
+#define MOD_VOLATILE		(1 << MOD_VOLATILE_BIT)
+#define MOD_SIGNED		(1 << MOD_SIGNED_BIT)
+#define MOD_UNSIGNED		(1 << MOD_UNSIGNED_BIT)
+
+#define MOD_CHAR		(1 << MOD_CHAR_BIT)
+#define MOD_SHORT		(1 << MOD_SHORT_BIT)
+#define MOD_LONG		(1 << MOD_LONG_BIT)
+#define MOD_LONGLONG		(1 << MOD_LONGLONG_BIT)
+
+#define MOD_TYPEDEF		(1 << MOD_TYPEDEF_BIT)
+#define MOD_WEAK		(1 << MOD_WEAK_BIT)
+
+#define MOD_INLINE		(1 << MOD_INLINE_BIT)
+#define MOD_ADDRESSABLE		(1 << MOD_ADDRESSABLE_BIT)
+
+#define MOD_NOCAST		(1 << MOD_NOCAST_BIT)
+#define MOD_NODEREF		(1 << MOD_NODEREF_BIT)
+#define MOD_ACCESSED		(1 << MOD_ACCESSED_BIT)
+#define MOD_TOPLEVEL		(1 << MOD_TOPLEVEL_BIT)
+
+#define MOD_LABEL		(1 << MOD_LABEL_BIT)
+#define MOD_ASSIGNED		(1 << MOD_ASSIGNED_BIT)
+#define MOD_TYPE		(1 << MOD_TYPE_BIT)
+#define MOD_SAFE		(1 << MOD_SAFE_BIT)
+
+#define MOD_USERTYPE		(1 << MOD_USERTYPE_BIT)
+#define MOD_FORCE		(1 << MOD_FORCE_BIT)
+#define MOD_EXPLICITLY_SIGNED	(1 << MOD_EXPLICITLY_SIGNED_BIT)
+#define MOD_BITWISE		(1 << MOD_BITWISE_BIT)
 
 #define MOD_NONLOCAL	(MOD_EXTERN | MOD_TOPLEVEL)
 #define MOD_STORAGE	(MOD_AUTO | MOD_REGISTER | MOD_STATIC | MOD_EXTERN | MOD_INLINE | MOD_TOPLEVEL | MOD_FORCE | MOD_WEAK)

-- 
mobile: +43 664 4416156              http://www.sysprog.at/
    Linux Software Development, Consulting and Services

Re: [PATCH] Fixup and cleanup of the modifier_string() function (was Re: modifier_string() inconsistency with modifiers)

[Christopher Li]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]

On Mon, Aug 23, 2010 at 7:42 AM, Bernd Petrovitsch <bernd@sysprog.at> wrote:
> Fixup and cleanup of the modifier_string() function
>
> This patch does:
> - it fixes the modifier_string() function
> - The array with the names is made "static".


Sorry for the late reply.

Your patch does not apply to the chrisl repository. I also notice that
if modifier
is zero, it will return buffer without initialized. Well the modifier
is not supposed
to be zero, but still.

Another thing in the patch is that I don't want to move the modifier
bits for just the debug
function. It is expected to have more change in the modifier bits. How about
this patch? I start with your patch and end up like this.

If that works for you, please sign it off and I will check it in.

Thanks,

Chris

[-- Attachment #2: mod-string.patch --]
[-- Type: application/octet-stream, Size: 2198 bytes --]

diff --git a/show-parse.c b/show-parse.c
index d7b502d..c97debe 100644
--- a/show-parse.c
+++ b/show-parse.c
@@ -95,29 +95,55 @@ void debug_symbol(struct symbol *sym)
 const char *modifier_string(unsigned long mod)
 {
 	static char buffer[100];
-	char *p = buffer;
-	const char *res,**ptr, *names[] = {
-		"auto", "register", "static", "extern",
-		"const", "volatile", "[signed]", "[unsigned]",
-		"[char]", "[short]", "[long]", "[long long]",
-		"[typedef]", "[structof]", "[unionof]", "[enum]",
-		"[typeof]", "[attribute]", "inline", "[addressable]",
-		"[nocast]", "[noderef]", "[accessed]", "[toplevel]",
-		"[label]", "[assigned]", "[type]", "[safe]",
-		"[usertype]", "[force]", "[explicitly-signed]",
-		NULL
+	int len = 0;
+	int i;
+	struct mod_name {
+		unsigned long mod;
+		const char *name;
+	} *m;
+
+	static struct mod_name mod_names[] = {
+		{MOD_AUTO,		"auto"},
+		{MOD_REGISTER,		"register"},
+		{MOD_STATIC,		"static"},
+		{MOD_EXTERN,		"extern"},
+		{MOD_CONST,		"const"},
+		{MOD_VOLATILE,		"volatile"},
+		{MOD_SIGNED,		"[signed]"},
+		{MOD_UNSIGNED,		"[unsigned]"},
+		{MOD_CHAR,		"[char]"},
+		{MOD_SHORT,		"[short]"},
+		{MOD_LONG,		"[long]"},
+		{MOD_LONGLONG,		"[long long]"},
+		{MOD_LONGLONGLONG,	"[long long long]"},
+		{MOD_TYPEDEF,		"[typedef]"},
+		{MOD_TLS,		"[tls]"},
+		{MOD_INLINE,		"inline"},
+		{MOD_ADDRESSABLE,	"[addressable]"},
+		{MOD_NOCAST,		"[nocast]"},
+		{MOD_NODEREF,		"[noderef]"},
+		{MOD_ACCESSED,		"[accessed]"},
+		{MOD_TOPLEVEL,		"[toplevel]"},
+		{MOD_ASSIGNED,		"[assigned]"},
+		{MOD_TYPE,		"[type]"},
+		{MOD_SAFE,		"[safe]"},
+		{MOD_USERTYPE,		"[usertype]"},
+		{MOD_NORETURN,		"[noreturn]"},
+		{MOD_EXPLICITLY_SIGNED,	"[explicitly-signed]"},
+		{MOD_BITWISE,		"[bitwise]"},
 	};
-	ptr = names;
-	while ((res = *ptr++) != NULL) {
-		if (mod & 1) {
+
+	for (i = 0; i < ARRAY_SIZE(mod_names); i++) {
+		m = mod_names + i;
+		if (mod & m->mod) {
 			char c;
-			while ((c = *res++) != '\0')
-				*p++ = c;
-			*p++ = ' ';
+			const char *name = m->name;
+			while ((c = *name++) != '\0' && len + 2 < sizeof buffer)
+				buffer[len++] = c;
+			buffer[len++] = ' ';
 		}
-		mod >>= 1;
 	}
-	*p = 0;
+	buffer[len] = 0;
 	return buffer;
 }
 

Re: [PATCH] Fixup and cleanup of the modifier_string() function

[Bernd Petrovitsch]

On Fre, 2010-09-03 at 02:13 -0700, Christopher Li wrote:
> On Mon, Aug 23, 2010 at 7:42 AM, Bernd Petrovitsch <bernd@sysprog.at> wrote:
> > Fixup and cleanup of the modifier_string() function
> >
> > This patch does:
> > - it fixes the modifier_string() function
> > - The array with the names is made "static".
> 
> Sorry for the late reply.

No problem - August is holiday time (at least hereover) and if there are
more pressing issues.

> Your patch does not apply to the chrisl repository. I also notice that

They were against git://git.kernel.org/pub/scm/devel/sparse/sparse.git
which is listed on https://sparse.wiki.kernel.org/index.php/Main_Page.
But your patch doesn't apply against it. And it doesn't apply against
git://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git - which
looks at the first glance identical to the above.

I found http://marc.info/?l=linux-sparse&m=126634898432123 but there is
no "chrisl" branch - at least not in the output of `git branch -a`.
What I'm doing wrong?

> if modifier
> is zero, it will return buffer without initialized. Well the modifier

Ooops, ACK.
Grrml, not the first time in my life that I forgot that.

> is not supposed
> to be zero, but still.
> 
> Another thing in the patch is that I don't want to move the modifier
> bits for just the debug
> function. It is expected to have more change in the modifier bits. How about
> this patch? I start with your patch and end up like this.

Yes, the associative array is the better solution.
And the buffer overflow is also fixed.

> If that works for you, please sign it off and I will check it in.

It works for me.
Signed-off-by: Bernd Petrovitsch <bernd@sysprog.at>

	Bernd
-- 
mobile: +43 664 4416156              http://www.sysprog.at/
    Linux Software Development, Consulting and Services

Re: [PATCH] Fixup and cleanup of the modifier_string() function

[Christopher Li]

On Tue, Sep 14, 2010 at 7:37 AM, Bernd Petrovitsch <bernd@sysprog.at> wrote:
> They were against git://git.kernel.org/pub/scm/devel/sparse/sparse.git
> which is listed on https://sparse.wiki.kernel.org/index.php/Main_Page.
> But your patch doesn't apply against it. And it doesn't apply against
> git://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git - which
> looks at the first glance identical to the above.
>

I think there is some thing wrong in your git setup. May be you did not
checkout the right branch? You can always visit the web link to see
if that match your local "git log".

http://git.kernel.org/?p=devel/sparse/chrisl/sparse.git;a=summary

> I found http://marc.info/?l=linux-sparse&m=126634898432123 but there is
> no "chrisl" branch - at least not in the output of `git branch -a`.
> What I'm doing wrong?

Have your try to clone a clean repository in a new directory?

git clone git://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git

That should give you a clean local copy of "chrisl" repository.

There is a better way to deal with remote branch in one git repository.
If you add those to your .git/config:

[remote "chrisl"]
        fetch = +refs/heads/*:refs/remotes/origin/*
        url = git://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git
[branch "chrisl"]
        remote = chrisl
        merge = refs/heads/master

You should be able to do:

git fetch chrisl
git checkout chrisl

To get the chrisl branch.

> Signed-off-by: Bernd Petrovitsch <bernd@sysprog.at>

Thanks. The change is pushed.

Chris