modifier_string() inconsistency with modifiers

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

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

Care for a patch?

Chris

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

Not at all. Sparse use c99 extensively.

Chris

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.

---
 show-parse.c |   63 +++++++++++++++++++++++-----------
 symbol.h     |  106 +++++++++++++++++++++++++++++++++++++++-------------------
 2 files changed, 114 insertions(+), 55 deletions(-)

index b771018..34227b4 100644
@@ -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)

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

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

index d7b502d..c97debe 100644
@@ -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;
 }
 

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

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?

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

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

It works for me.

Bernd

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

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

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.

Thanks. The change is pushed.

Chris