[PATCH] libtracefs: Fix sometimes uninitialized warning.

[PATCH] libtracefs: Fix sometimes uninitialized warning.

[Ian Rogers]

The warning with clang looks like:

src/tracefs-sqlhist.c:1107:2: error: variable 'cmp' is used uninitialized whenever switch default is taken [-Werror,-Wsometimes-uninitialized]
        default:
        ^~~~~~~
third_party/libtracefs/src/tracefs-sqlhist.c:1112:35: note: uninitialized use occurs here
                            filter->lval->field.field, cmp, val);
                                                       ^~~
third_party/libtracefs/src/tracefs-sqlhist.c:1033:2: note: variable 'cmp' is declared here
        enum tracefs_compare cmp;

Signed-off-by: Ian Rogers <irogers@google.com>
---
 src/tracefs-sqlhist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/tracefs-sqlhist.c b/src/tracefs-sqlhist.c
index 6224677..f4dc004 100644
--- a/src/tracefs-sqlhist.c
+++ b/src/tracefs-sqlhist.c
@@ -1105,7 +1105,7 @@ static int build_filter(struct tep_handle *tep, struct sqlhist_bison *sb,
 	case FILTER_BIN_AND:	cmp = TRACEFS_COMPARE_AND; break;
 	case FILTER_STR_CMP:	cmp = TRACEFS_COMPARE_RE; break;
 	default:
-		break;
+		abort();
 	}
 
 	ret = append_filter(synth, TRACEFS_FILTER_COMPARE,
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH] libtracefs: Fix sometimes uninitialized warning.

[Steven Rostedt]

On Thu,  2 Sep 2021 15:48:40 -0700
Ian Rogers <irogers@google.com> wrote:

> The warning with clang looks like:
> 
> src/tracefs-sqlhist.c:1107:2: error: variable 'cmp' is used uninitialized whenever switch default is taken [-Werror,-Wsometimes-uninitialized]
>         default:
>         ^~~~~~~
> third_party/libtracefs/src/tracefs-sqlhist.c:1112:35: note: uninitialized use occurs here
>                             filter->lval->field.field, cmp, val);
>                                                        ^~~
> third_party/libtracefs/src/tracefs-sqlhist.c:1033:2: note: variable 'cmp' is declared here
>         enum tracefs_compare cmp;
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  src/tracefs-sqlhist.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/tracefs-sqlhist.c b/src/tracefs-sqlhist.c
> index 6224677..f4dc004 100644
> --- a/src/tracefs-sqlhist.c
> +++ b/src/tracefs-sqlhist.c
> @@ -1105,7 +1105,7 @@ static int build_filter(struct tep_handle *tep, struct sqlhist_bison *sb,
>  	case FILTER_BIN_AND:	cmp = TRACEFS_COMPARE_AND; break;
>  	case FILTER_STR_CMP:	cmp = TRACEFS_COMPARE_RE; break;
>  	default:
> -		break;
> +		abort();

Hmm, I wonder if we should do something different than call abort().

Although this should never happen, I think it's rather rude of a library
function to call abort() when it fails.

I'd like to see this simply return an error. At most, a warning can be
printed.

Thanks!

-- Steve


>  	}
>  
>  	ret = append_filter(synth, TRACEFS_FILTER_COMPARE,

Re: [PATCH] libtracefs: Fix sometimes uninitialized warning.

[Ian Rogers]

On Tue, Sep 7, 2021 at 7:57 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu,  2 Sep 2021 15:48:40 -0700
> Ian Rogers <irogers@google.com> wrote:
>
> > The warning with clang looks like:
> >
> > src/tracefs-sqlhist.c:1107:2: error: variable 'cmp' is used uninitialized whenever switch default is taken [-Werror,-Wsometimes-uninitialized]
> >         default:
> >         ^~~~~~~
> > third_party/libtracefs/src/tracefs-sqlhist.c:1112:35: note: uninitialized use occurs here
> >                             filter->lval->field.field, cmp, val);
> >                                                        ^~~
> > third_party/libtracefs/src/tracefs-sqlhist.c:1033:2: note: variable 'cmp' is declared here
> >         enum tracefs_compare cmp;
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  src/tracefs-sqlhist.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/tracefs-sqlhist.c b/src/tracefs-sqlhist.c
> > index 6224677..f4dc004 100644
> > --- a/src/tracefs-sqlhist.c
> > +++ b/src/tracefs-sqlhist.c
> > @@ -1105,7 +1105,7 @@ static int build_filter(struct tep_handle *tep, struct sqlhist_bison *sb,
> >       case FILTER_BIN_AND:    cmp = TRACEFS_COMPARE_AND; break;
> >       case FILTER_STR_CMP:    cmp = TRACEFS_COMPARE_RE; break;
> >       default:
> > -             break;
> > +             abort();
>
> Hmm, I wonder if we should do something different than call abort().
>
> Although this should never happen, I think it's rather rude of a library
> function to call abort() when it fails.
>
> I'd like to see this simply return an error. At most, a warning can be
> printed.
>
> Thanks!

Thanks Steve, I think the error handling in this function could use
some TLC. For example, the code:

ret = append_filter(synth, and_or, NULL, 0, NULL);
ret = append_filter(synth, TRACEFS_FILTER_OPEN_PAREN,
    NULL, 0, NULL);

doesn't check the value of ret. Assuming the return values are checked
then plumbing these up makes sense. Fwiw, there is evidence that going
in the direction of asserts/aborts is useful:
https://www.microsoft.com/en-us/research/publication/assessing-the-relationship-between-software-assertions-and-code-qualityan-empirical-investigation/

I can resend the patch with something like:

fprintf(stderr, "Error invalid filter type '%d'", filter->type);
return ERANGE;

Does that fit the expected convention?

Thanks!
Ian

> -- Steve
>
>
> >       }
> >
> >       ret = append_filter(synth, TRACEFS_FILTER_COMPARE,
>

Re: [PATCH] libtracefs: Fix sometimes uninitialized warning.

[Steven Rostedt]

On Tue, 7 Sep 2021 11:53:48 -0700
Ian Rogers <irogers@google.com> wrote:

> Thanks Steve, I think the error handling in this function could use
> some TLC. For example, the code:

100% agree. That's one reason I haven't tagged an official release with it
yet ;-)


> 
> ret = append_filter(synth, and_or, NULL, 0, NULL);
> ret = append_filter(synth, TRACEFS_FILTER_OPEN_PAREN,
>     NULL, 0, NULL);
> 
> doesn't check the value of ret. Assuming the return values are checked
> then plumbing these up makes sense. Fwiw, there is evidence that going
> in the direction of asserts/aborts is useful:
> https://www.microsoft.com/en-us/research/publication/assessing-the-relationship-between-software-assertions-and-code-qualityan-empirical-investigation/

I'm sure it is. But I still hate it when a library aborts my own work, so
I'm biased ;-)

> 
> I can resend the patch with something like:
> 
> fprintf(stderr, "Error invalid filter type '%d'", filter->type);
> return ERANGE;
> 
> Does that fit the expected convention?

Looks as good as any.

If you want to send patches that makes the return handling a bit more
complete and robust, that would be more than welcomed.

-- Steve