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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
<[email protected]> 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
Hi Andy,
On Sat, Jun 28, 2014 at 9:59 AM, Christopher Li <[email protected]> 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
On Sun, Jun 29, 2014 at 12:19 AM, Christopher Li <[email protected]> wrote:
>
> If no objections, I will push the change.
>
Change pushed.
Chris
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
> <[email protected]> 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 <[email protected]>
Intel Finland Oy
On Mon, Jun 30, 2014 at 1:32 AM, Andy Shevchenko
<[email protected]> 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
On Mon, 2014-06-30 at 01:51 -0700, Christopher Li wrote:
> On Mon, Jun 30, 2014 at 1:32 AM, Andy Shevchenko
> <[email protected]> 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 <[email protected]>
Intel Finland Oy