| Message ID | 20210902224840.2518539-1-irogers@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libtracefs: Fix sometimes uninitialized warning. | expand |
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,
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, >
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
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,
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(-)