[PATCH v2 0/2] fix for --param GCC's command line option

[PATCH v2 0/2] fix for --param GCC's command line option

[Andy Shevchenko]

There is a dumb fix to avoid --param option and make sparse survive.

Since v1:
 - added patch 1/2
 - handle both --param=* and --param *

Andy Shevchenko (2):
  lib.c: introduce split_value_from_arg helper
  lib.c: skip --param parameters

 lib.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

-- 
2.0.0

[PATCH v2 1/2] lib.c: introduce split_value_from_arg helper

[Andy Shevchenko]

The function tries to split a key / value from the given argument where
delimiter can be either ' ' (space) or '=' (equal sign).

It will be useful later as well.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/lib.c b/lib.c
index bf3e91c..4e5a846 100644
--- a/lib.c
+++ b/lib.c
@@ -275,14 +275,8 @@ void add_pre_buffer(const char *fmt, ...)
 	pre_buffer_end = end;
 }
 
-static char **handle_switch_D(char *arg, char **next)
+static const char *split_value_from_arg(char *arg, const char *def)
 {
-	const char *name = arg + 1;
-	const char *value = "1";
-
-	if (!*name || isspace(*name))
-		die("argument to `-D' is missing");
-
 	for (;;) {
 		char c;
 		c = *++arg;
@@ -290,10 +284,21 @@ static char **handle_switch_D(char *arg, char **next)
 			break;
 		if (isspace((unsigned char)c) || c == '=') {
 			*arg = '\0';
-			value = arg + 1;
-			break;
+			return arg + 1;
 		}
 	}
+	return def;
+}
+
+static char **handle_switch_D(char *arg, char **next)
+{
+	const char *name = arg + 1;
+	const char *value = "1";
+
+	if (!*name || isspace(*name))
+		die("argument to `-D' is missing");
+
+	value = split_value_from_arg(arg, value);
 	add_pre_buffer("#define %s %s\n", name, value);
 	return next;
 }
-- 
2.0.0

[PATCH v2 2/2] lib.c: skip --param parameters

[Andy Shevchenko]

Very dumb patch to just skip --param allow-store-data-races=0 introduced in
newer GCC versions.

Without this patch sparse recognizes parameter of the --param option as a file
name which obviously couldn't be found.

The patch for easy implementation's sake slightly changed behaviour of
--version. Instead of exact keyword it will recognize anything starting with
--version as a correct option.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/lib.c b/lib.c
index 4e5a846..d5b94c3 100644
--- a/lib.c
+++ b/lib.c
@@ -678,6 +678,18 @@ static char **handle_version(char *arg, char **next)
 	exit(0);
 }
 
+static char **handle_param(char *arg, char **next)
+{
+	const char *value = NULL;
+
+	/* For now just skip any '--param=*' or '--param *' */
+	value = split_value_from_arg(arg, value);
+	if (!value)
+		++next;
+
+	return ++next;
+}
+
 struct switches {
 	const char *name;
 	char **(*fn)(char *, char **);
@@ -686,13 +698,14 @@ struct switches {
 static char **handle_long_options(char *arg, char **next)
 {
 	static struct switches cmd[] = {
+		{ "param", handle_param },
 		{ "version", handle_version },
 		{ NULL, NULL }
 	};
 	struct switches *s = cmd;
 
 	while (s->name) {
-		if (!strcmp(s->name, arg))
+		if (!strncmp(arg, s->name, strlen(s->name)))
 			return s->fn(arg, next);
 		s++;
 	}
-- 
2.0.0

Re: [PATCH v2 2/2] lib.c: skip --param parameters

[Christopher Li]

Re: [PATCH v2 2/2] lib.c: skip --param parameters

[Christopher Li]

Oops, I just click send before I type up the reply. Here we go again.

On Tue, Jun 17, 2014 at 2:11 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Very dumb patch to just skip --param allow-store-data-races=0 introduced in
> newer GCC versions.
>
> +static char **handle_param(char *arg, char **next)
> +{
> +       const char *value = NULL;
> +
> +       /* For now just skip any '--param=*' or '--param *' */
> +       value = split_value_from_arg(arg, value);
> +       if (!value)
> +               ++next;
> +
> +       return ++next;
> +}

I think this is problematic.There are three possible input
from args:
1) "--parm", you need to ++next skip to next arg, which is the value for parm.
2) "--parm=x",  you don't need to skip to next arg.
3) "--parm-with-crap", invalid argument. You don't need to skip next arg.

I think the patch is wrong on case 2) and case 3).
In case 2), the patch skip two arguments and make next point
points to out of bound memory.

The split_value_from_arg function is not a good abstraction for this job.
Its return value can only indicate 2 possible out come.
Also, returning the default value force the test against the input
default value. That make the logic a bit complicate.

>  struct switches {
>         const char *name;
>         char **(*fn)(char *, char **);
> @@ -686,13 +698,14 @@ struct switches {
>  static char **handle_long_options(char *arg, char **next)
>  {
>         static struct switches cmd[] = {
> +               { "param", handle_param },
>                 { "version", handle_version },
>                 { NULL, NULL }
>         };
>         struct switches *s = cmd;
>
>         while (s->name) {
> -               if (!strcmp(s->name, arg))
> +               if (!strncmp(arg, s->name, strlen(s->name)))

This will allow "--version-with-crap" as valid arguments.

I think we can have one extra member in "struct switch"
to indicate this option is a prefix rather than a whole word.
For "parm", it need to set that prefix member to non zero.

Please let me know if there is a V3 coming.

Chris

Re: [PATCH v2 2/2] lib.c: skip --param parameters

[Christopher Li]

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

Hi Andy,

On Sat, Jun 28, 2014 at 9:59 AM, Christopher Li <sparse@chrisli.org> wrote:
> I think this is problematic.There are three possible input
> from args:
> 1) "--parm", you need to ++next skip to next arg, which is the value for parm.
> 2) "--parm=x",  you don't need to skip to next arg.
> 3) "--parm-with-crap", invalid argument. You don't need to skip next arg.


How about this patch, I modify from your patch.
It fix the problem I mention earlier.

If no objections, I will push the change.

Chris

[-- Attachment #2: 0001-lib.c-skip-param-parameters.patch --]
[-- Type: text/x-patch, Size: 1705 bytes --]

From d917662d54ba68d0c3b03e994cb1fa66d7b19c30 Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Tue, 17 Jun 2014 12:11:45 +0300
Subject: [PATCH] lib.c: skip --param parameters

Very dumb patch to just skip --param allow-store-data-races=0 introduced in
newer GCC versions.

Without this patch sparse recognizes parameter of the --param option as a file
name which obviously couldn't be found.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Christopher Li <sparse@chrisli.org>
---
 lib.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/lib.c b/lib.c
index 844797d..0bc4b2b 100644
--- a/lib.c
+++ b/lib.c
@@ -675,22 +675,42 @@ static char **handle_version(char *arg, char **next)
 	exit(0);
 }
 
+static char **handle_param(char *arg, char **next)
+{
+	char *value = NULL;
+
+	/* For now just skip any '--param=*' or '--param *' */
+	if (*arg == '\0') {
+		value = *++next;
+	} else if (isspace(*arg) || *arg == '=') {
+		value = ++arg;
+	}
+
+	if (!value)
+		die("missing argument for --param option");
+
+	return next;
+}
+
 struct switches {
 	const char *name;
 	char **(*fn)(char *, char **);
+	unsigned int prefix:1;
 };
 
 static char **handle_long_options(char *arg, char **next)
 {
 	static struct switches cmd[] = {
+		{ "param", handle_param, 1 },
 		{ "version", handle_version },
 		{ NULL, NULL }
 	};
 	struct switches *s = cmd;
 
 	while (s->name) {
-		if (!strcmp(s->name, arg))
-			return s->fn(arg, next);
+		int optlen = strlen(s->name);
+		if (!strncmp(s->name, arg, optlen + !s->prefix))
+			return s->fn(arg + optlen, next);
 		s++;
 	}
 	return next;
-- 
1.9.3

Re: [PATCH v2 2/2] lib.c: skip --param parameters

[Christopher Li]

On Sun, Jun 29, 2014 at 12:19 AM, Christopher Li <sparse@chrisli.org> wrote:
>
> If no objections, I will push the change.
>

Change pushed.

Chris

Re: [PATCH v2 2/2] lib.c: skip --param parameters

[Andy Shevchenko]

On Sat, 2014-06-28 at 09:59 -0700, Christopher Li wrote:
> Oops, I just click send before I type up the reply. Here we go again.
> 
> On Tue, Jun 17, 2014 at 2:11 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Very dumb patch to just skip --param allow-store-data-races=0 introduced in
> > newer GCC versions.
> >
> > +static char **handle_param(char *arg, char **next)
> > +{
> > +       const char *value = NULL;
> > +
> > +       /* For now just skip any '--param=*' or '--param *' */
> > +       value = split_value_from_arg(arg, value);
> > +       if (!value)
> > +               ++next;
> > +
> > +       return ++next;
> > +}
> 
> I think this is problematic.There are three possible input
> from args:
> 1) "--parm", you need to ++next skip to next arg, which is the value for parm.
> 2) "--parm=x",  you don't need to skip to next arg.
> 3) "--parm-with-crap", invalid argument. You don't need to skip next arg.
> 
> I think the patch is wrong on case 2) and case 3).
> In case 2), the patch skip two arguments and make next point
> points to out of bound memory.

Hmm... I'd just added test printf to the handle_param() and see if I
print *next, it is either --param or --param=*. So, using return (next +
2) helps, otherwise we end up with the same situation as before patch.

What did I miss?

> 
> The split_value_from_arg function is not a good abstraction for this job.
> Its return value can only indicate 2 possible out come.
> Also, returning the default value force the test against the input
> default value. That make the logic a bit complicate.
> 
> >  struct switches {
> >         const char *name;
> >         char **(*fn)(char *, char **);
> > @@ -686,13 +698,14 @@ struct switches {
> >  static char **handle_long_options(char *arg, char **next)
> >  {
> >         static struct switches cmd[] = {
> > +               { "param", handle_param },
> >                 { "version", handle_version },
> >                 { NULL, NULL }
> >         };
> >         struct switches *s = cmd;
> >
> >         while (s->name) {
> > -               if (!strcmp(s->name, arg))
> > +               if (!strncmp(arg, s->name, strlen(s->name)))
> 
> This will allow "--version-with-crap" as valid arguments.

Which was explicitly mentioned in the commit message.

> 
> I think we can have one extra member in "struct switch"
> to indicate this option is a prefix rather than a whole word.
> For "parm", it need to set that prefix member to non zero.

No objections about this approach.

> Please let me know if there is a V3 coming.

Apparently you did this on weekend.


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH v2 2/2] lib.c: skip --param parameters

[Christopher Li]

On Mon, Jun 30, 2014 at 1:32 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Hmm... I'd just added test printf to the handle_param() and see if I
> print *next, it is either --param or --param=*. So, using return (next +
> 2) helps, otherwise we end up with the same situation as before patch.

The return value from handle_switch() is a bit tricky. It is actually points to
the current args which about to be expired.

Take a look at this code which invoke the handle_switch().
    for (;;) {
        char *arg = *++args;      <---------------- notice the ++
before the fetch
        if (!arg)
            break;

        if (arg[0] == '-' && arg[1]) {
            args = handle_switch(arg+1, args); <-------- args return here.
            continue;
        }
        add_ptr_list_notag(filelist, arg);
    }

>
> What did I miss?

So the caller loop will perform 1 pointer advance before fetch.
Your code can advance 2 pointer, so that is  total 3 pointer advance.

>
> Which was explicitly mentioned in the commit message.

Sorry about that, I jump to the code first. I later notice that  in
the commit message as well.

Any way, the change I push should fix all that.

Chris

Re: [PATCH v2 2/2] lib.c: skip --param parameters

[Andy Shevchenko]

On Mon, 2014-06-30 at 01:51 -0700, Christopher Li wrote:
> On Mon, Jun 30, 2014 at 1:32 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Hmm... I'd just added test printf to the handle_param() and see if I
> > print *next, it is either --param or --param=*. So, using return (next +
> > 2) helps, otherwise we end up with the same situation as before patch.
> 
> The return value from handle_switch() is a bit tricky. It is actually points to
> the current args which about to be expired.
> 
> Take a look at this code which invoke the handle_switch().
>     for (;;) {
>         char *arg = *++args;      <---------------- notice the ++
> before the fetch
>         if (!arg)
>             break;
> 
>         if (arg[0] == '-' && arg[1]) {
>             args = handle_switch(arg+1, args); <-------- args return here.
>             continue;
>         }
>         add_ptr_list_notag(filelist, arg);
>     }
> 
> >
> > What did I miss?
> 
> So the caller loop will perform 1 pointer advance before fetch.
> Your code can advance 2 pointer, so that is  total 3 pointer advance.

Yeah, thanks for explanation. Just noticed this after send a message.

> 
> >
> > Which was explicitly mentioned in the commit message.
> 
> Sorry about that, I jump to the code first. I later notice that  in
> the commit message as well.
> 
> Any way, the change I push should fix all that.

Yup. Thank you.


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy