| Message ID | a6b73c54-3010-6716-cac3-8f3b462a4dc7@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [4.15] gnttab: work around "may be used uninitialized" warning | expand |
Hi Jan, On 10/03/2021 10:13, Jan Beulich wrote: > Sadly I was wrong to suggest dropping vaddrs' initializer during review > of v2 of the patch introducing this code. gcc 4.3 can't cope. What's the error? Are you sure this is not going to hiding a potential miscompilation of the function? > > Fixes: 52531c734ea1 ("xen/gnttab: Rework resource acquisition") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -4026,7 +4026,7 @@ int gnttab_acquire_resource( > struct grant_table *gt = d->grant_table; > unsigned int i, final_frame; > mfn_t tmp; > - void **vaddrs; > + void **vaddrs = NULL; I am a bit nervous to inialize vaddrs to NULL for a few reasons: 1) It is not 100% obvious (e.g. it takes more than a second) that vaddrs will always be initialized. 2) A compiler will not be able to help us if we are adding code without initialized vaddrs. It also feels wrong to me to try to write Xen in a way that will make a 10 years compiler happy... If we still want to support them, then maybe a better approach would be to turn off to turn off -Werror for some version of GCC. So we can continue to benefit from the newer compiler diagnostics. Cheers,
On 10.03.2021 15:58, Julien Grall wrote: > On 10/03/2021 10:13, Jan Beulich wrote: >> Sadly I was wrong to suggest dropping vaddrs' initializer during review >> of v2 of the patch introducing this code. gcc 4.3 can't cope. > > What's the error? The one quoted in the title. > Are you sure this is not going to hiding a potential > miscompilation of the function? Miscompilation? It may hide us screwing up, but addressing such a compiler warning by adding an initializer has been quite common in the past. >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -4026,7 +4026,7 @@ int gnttab_acquire_resource( >> struct grant_table *gt = d->grant_table; >> unsigned int i, final_frame; >> mfn_t tmp; >> - void **vaddrs; >> + void **vaddrs = NULL; > I am a bit nervous to inialize vaddrs to NULL for a few reasons: > 1) It is not 100% obvious (e.g. it takes more than a second) that > vaddrs will always be initialized. But convincing ourselves was necessary even more so prior to this change. We must not ever rely on the compiler to tell us about issues in our code. We can only leverage that in some cases it does. From this it (I think obviously) follows that without the initializer we're at bigger risk than with it. > 2) A compiler will not be able to help us if we are adding code > without initialized vaddrs. > > It also feels wrong to me to try to write Xen in a way that will make a > 10 years compiler happy... As said above - we've worked around limitations quite a few times in the past. This is just one more instance. > If we still want to support them, then maybe a better approach would be > to turn off to turn off -Werror for some version of GCC. So we can > continue to benefit from the newer compiler diagnostics. Avoiding use of -Werror is not an option imo: Once you start seeing warnings, you have only two options imo: Either one decides to ignore them all (and then one will also ignore new ones introduce by changes yet to be submitted), or one would have to memorize, for every build one does, which warnings one ought to ignore. The latter doesn't scale, while the former is a code quality problem. Suppressing a particular class of warning might be an option, but again risks somebody submitting code which elsewhere would trigger warnings. Jan
Hi Jan, On 10/03/2021 16:21, Jan Beulich wrote: > On 10.03.2021 15:58, Julien Grall wrote: >> On 10/03/2021 10:13, Jan Beulich wrote: >>> Sadly I was wrong to suggest dropping vaddrs' initializer during review >>> of v2 of the patch introducing this code. gcc 4.3 can't cope. >> >> What's the error? > > The one quoted in the title. > >> Are you sure this is not going to hiding a potential >> miscompilation of the function? > > Miscompilation? It may hide us screwing up, but addressing such a > compiler warning by adding an initializer has been quite common > in the past. Well... When a compiler tells me a value may be unitialized, I read it as some optimization may decide to use the variable in a way I wasn't expected. >>> --- a/xen/common/grant_table.c >>> +++ b/xen/common/grant_table.c >>> @@ -4026,7 +4026,7 @@ int gnttab_acquire_resource( >>> struct grant_table *gt = d->grant_table; >>> unsigned int i, final_frame; >>> mfn_t tmp; >>> - void **vaddrs; >>> + void **vaddrs = NULL; >> I am a bit nervous to inialize vaddrs to NULL for a few reasons: >> 1) It is not 100% obvious (e.g. it takes more than a second) that >> vaddrs will always be initialized. > > But convincing ourselves was necessary even more so prior to this > change. We must not ever rely on the compiler to tell us about > issues in our code. We can only leverage that in some cases it > does. I didn't suggest that we should only rely on the compiler. I pointed out that we are telling the compiler to not worry. This may hide a valid concern from the compiler. > From this it (I think obviously) follows that without the > initializer we're at bigger risk than with it. I thought deferencing a NULL pointer was still a concern for PV? For the other setup, I agree that it would only lead to a crash rather than dereferencing anything. Yet I am not convinced this is that much better... >> 2) A compiler will not be able to help us if we are adding code >> without initialized vaddrs. >> >> It also feels wrong to me to try to write Xen in a way that will make a >> 10 years compiler happy... > > As said above - we've worked around limitations quite a few times > in the past. This is just one more instance. I find amusing you wrote that when you complained multiple time when someone was re-using existing bad pattern. :) > >> If we still want to support them, then maybe a better approach would be >> to turn off to turn off -Werror for some version of GCC. So we can >> continue to benefit from the newer compiler diagnostics. > > Avoiding use of -Werror is not an option imo: Once you start seeing > warnings, you have only two options imo: Either one decides to ignore > them all (and then one will also ignore new ones introduce by changes > yet to be submitted), or one would have to memorize, for every build > one does, which warnings one ought to ignore. The latter doesn't > scale, while the former is a code quality problem. > > Suppressing a particular class of warning might be an option, but > again risks somebody submitting code which elsewhere would trigger > warnings. This is pretty much what we are already doing slowly by initializing values to shut up older compilers. I agree this is more limited, but we also waive off diagnostics from every single compiler in that code rather than just one version. Hence why I suggested dropping -Werror for older compiler. This is not ideal but it would give us the ability to keep support for dinausor compiler and not hamper our ability to take advantage of newer compiler diagnostics. The ideal solution is to drop support for older compiler (see my other thread). But this sounds like a daunting task so far on x86... Anyway, I will not Nack the patch but will also not Ack it. I will let another maintainer ack this patch. Cheers,
On 10.03.2021 18:52, Julien Grall wrote: > On 10/03/2021 16:21, Jan Beulich wrote: >> On 10.03.2021 15:58, Julien Grall wrote: >>> On 10/03/2021 10:13, Jan Beulich wrote: >>>> Sadly I was wrong to suggest dropping vaddrs' initializer during review >>>> of v2 of the patch introducing this code. gcc 4.3 can't cope. >>> >>> What's the error? >> >> The one quoted in the title. >> >>> Are you sure this is not going to hiding a potential >>> miscompilation of the function? >> >> Miscompilation? It may hide us screwing up, but addressing such a >> compiler warning by adding an initializer has been quite common >> in the past. > > Well... When a compiler tells me a value may be unitialized, I read it > as some optimization may decide to use the variable in a way I wasn't > expected. I don't think that's how warnings like this work in general. Optimization may help spot a case where an uninitialized variable gets used (because optimization may result in sufficient simplification of the internal representation of the original source), and variable lifetime analysis may also be a prereq to optimization, but in general I'd recommend viewing the two as independent aspects. >>>> --- a/xen/common/grant_table.c >>>> +++ b/xen/common/grant_table.c >>>> @@ -4026,7 +4026,7 @@ int gnttab_acquire_resource( >>>> struct grant_table *gt = d->grant_table; >>>> unsigned int i, final_frame; >>>> mfn_t tmp; >>>> - void **vaddrs; >>>> + void **vaddrs = NULL; >>> I am a bit nervous to inialize vaddrs to NULL for a few reasons: >>> 1) It is not 100% obvious (e.g. it takes more than a second) that >>> vaddrs will always be initialized. >> >> But convincing ourselves was necessary even more so prior to this >> change. We must not ever rely on the compiler to tell us about >> issues in our code. We can only leverage that in some cases it >> does. > > I didn't suggest that we should only rely on the compiler. I pointed out > that we are telling the compiler to not worry. This may hide a valid > concern from the compiler. As we (have to) do elsewhere. >> From this it (I think obviously) follows that without the >> initializer we're at bigger risk than with it. > > I thought deferencing a NULL pointer was still a concern for PV? I could use ZERO_BLOCK_PTR or yet something else, if we decided we want to work around this class of issues this way. Elsewhere we're using NULL afaict (but see also below). > For the other setup, I agree that it would only lead to a crash rather > than dereferencing anything. Yet I am not convinced this is that much > better... When using an uninitialized variable, anything can happen. A crash would still be on the better side of things, as a privilege escalation then also is possible. Again - if you're worried about us introducing an actual use of the thus initialized variable, you should be even more worried about using it when it's uninitialized (and the compiler _not_ being able to spot it). >>> 2) A compiler will not be able to help us if we are adding code >>> without initialized vaddrs. >>> >>> It also feels wrong to me to try to write Xen in a way that will make a >>> 10 years compiler happy... >> >> As said above - we've worked around limitations quite a few times >> in the past. This is just one more instance. > > I find amusing you wrote that when you complained multiple time when > someone was re-using existing bad pattern. :) Well, thing is - I don't view this as a bad pattern. The only question really is whether NULL is a good initializer here. As per above a non- canonical pointer may be better, but then we have quite a few places elsewhere to fix. We could also deliberately leave the variable uninitialized, by using past Linux'es uninitialized_var(), but they've dropped that construct for what I think are very good reasons. Jan
On Thu, Mar 11, 2021 at 09:09:22AM +0100, Jan Beulich wrote: > On 10.03.2021 18:52, Julien Grall wrote: > > On 10/03/2021 16:21, Jan Beulich wrote: > >> On 10.03.2021 15:58, Julien Grall wrote: > >>> On 10/03/2021 10:13, Jan Beulich wrote: > >>> 2) A compiler will not be able to help us if we are adding code > >>> without initialized vaddrs. > >>> > >>> It also feels wrong to me to try to write Xen in a way that will make a > >>> 10 years compiler happy... > >> > >> As said above - we've worked around limitations quite a few times > >> in the past. This is just one more instance. > > > > I find amusing you wrote that when you complained multiple time when > > someone was re-using existing bad pattern. :) > > Well, thing is - I don't view this as a bad pattern. The only question > really is whether NULL is a good initializer here. As per above a non- > canonical pointer may be better, but then we have quite a few places > elsewhere to fix. Sorry for jumping in the middle but I think that would be a very dangerous move for Xen to do. We have been using implicit conversions of pointers to booleans all over the place, assuming that NULL == false, hence NULL no longer mapping to false would break a lot of our code. ie: if ( foo ) free(foo); Would no longer work as expected. Thanks, Roger.
On 11.03.2021 09:25, Roger Pau Monné wrote: > On Thu, Mar 11, 2021 at 09:09:22AM +0100, Jan Beulich wrote: >> On 10.03.2021 18:52, Julien Grall wrote: >>> On 10/03/2021 16:21, Jan Beulich wrote: >>>> On 10.03.2021 15:58, Julien Grall wrote: >>>>> On 10/03/2021 10:13, Jan Beulich wrote: >>>>> 2) A compiler will not be able to help us if we are adding code >>>>> without initialized vaddrs. >>>>> >>>>> It also feels wrong to me to try to write Xen in a way that will make a >>>>> 10 years compiler happy... >>>> >>>> As said above - we've worked around limitations quite a few times >>>> in the past. This is just one more instance. >>> >>> I find amusing you wrote that when you complained multiple time when >>> someone was re-using existing bad pattern. :) >> >> Well, thing is - I don't view this as a bad pattern. The only question >> really is whether NULL is a good initializer here. As per above a non- >> canonical pointer may be better, but then we have quite a few places >> elsewhere to fix. > > Sorry for jumping in the middle but I think that would be a very > dangerous move for Xen to do. We have been using implicit conversions > of pointers to booleans all over the place, assuming that NULL == > false, hence NULL no longer mapping to false would break a lot of our > code. ie: > > if ( foo ) > free(foo); > > Would no longer work as expected. Funny you should give this example: Assuming you mean xfree(), it specifically tolerates both NULL and ZERO_BLOCK_PTR (the latter because xmalloc() may return it, and that's what it was invented for). But yes - other non-NULL checking guards would indeed break. I'm afraid every possible solution here has its downsides, and the suggested change simply follows the pattern we used so far in similar cases - without anyone objecting. Jan
Julien Grall writes ("Re: [PATCH][4.15] gnttab: work around "may be used uninitialized" warning"):
> This is pretty much what we are already doing slowly by initializing
> values to shut up older compilers. I agree this is more limited, but we
> also waive off diagnostics from every single compiler in that code
> rather than just one version.
>
> Hence why I suggested dropping -Werror for older compiler. This is not
> ideal but it would give us the ability to keep support for dinausor
> compiler and not hamper our ability to take advantage of newer compiler
> diagnostics.
I agree with Julien. I think we should avoid adding these redundant
initialisers for the reasons he gives.
> The ideal solution is to drop support for older compiler (see my other
> thread). But this sounds like a daunting task so far on x86...
>
> Anyway, I will not Nack the patch but will also not Ack it. I will let
> another maintainer ack this patch.
But this is outside my usual area so I won't nack it either.
Ian.
On 12.03.2021 11:04, Ian Jackson wrote: > Julien Grall writes ("Re: [PATCH][4.15] gnttab: work around "may be used uninitialized" warning"): >> This is pretty much what we are already doing slowly by initializing >> values to shut up older compilers. I agree this is more limited, but we >> also waive off diagnostics from every single compiler in that code >> rather than just one version. >> >> Hence why I suggested dropping -Werror for older compiler. This is not >> ideal but it would give us the ability to keep support for dinausor >> compiler and not hamper our ability to take advantage of newer compiler >> diagnostics. > > I agree with Julien. I think we should avoid adding these redundant > initialisers for the reasons he gives. I find this odd, not only because it is contrary to what we've done so far. What if more modern gcc issues a false-positive warning? If we'd fix it there, where would you suggest to draw the line? Imo our tree should build without issues on all compiler versions which we state we permit to be used. Of course in the case here I could add a "default:" to the switch(), but this would still only work around the compiler issue. Would the two of you consider this any better? Also, Ian - do you have any alternative suggestion towards making the build work again (in the more general case, i.e. irrespective of the alternative suggestion for this specific case just above)? Not using -Werror on old compilers (again - where would we draw the line) was already objected to by me. Jan
Jan Beulich writes ("Re: [PATCH][4.15] gnttab: work around "may be used uninitialized" warning"):
> On 12.03.2021 11:04, Ian Jackson wrote:
> > Julien Grall writes ("Re: [PATCH][4.15] gnttab: work around "may be used uninitialized" warning"):
> >> This is pretty much what we are already doing slowly by initializing
> >> values to shut up older compilers. I agree this is more limited, but we
> >> also waive off diagnostics from every single compiler in that code
> >> rather than just one version.
> >>
> >> Hence why I suggested dropping -Werror for older compiler. This is not
> >> ideal but it would give us the ability to keep support for dinausor
> >> compiler and not hamper our ability to take advantage of newer compiler
> >> diagnostics.
> >
> > I agree with Julien. I think we should avoid adding these redundant
> > initialisers for the reasons he gives.
>
> I find this odd, not only because it is contrary to what we've done so
> far. What if more modern gcc issues a false-positive warning? If we'd
> fix it there, where would you suggest to draw the line? Imo our tree
> should build without issues on all compiler versions which we state we
> permit to be used.
>
> Of course in the case here I could add a "default:" to the switch(),
> but this would still only work around the compiler issue. Would the
> two of you consider this any better?
>
> Also, Ian - do you have any alternative suggestion towards making the
> build work again (in the more general case, i.e. irrespective of the
> alternative suggestion for this specific case just above)? Not using
> -Werror on old compilers (again - where would we draw the line) was
> already objected to by me.
I read your objection to not using -Werror for such old compilers but
I did not agree with it.
I am sympathetic to Julien's desire to try to limit the set of
supported compilers.
Ian.
On 12.03.2021 11:29, Ian Jackson wrote: > I am sympathetic to Julien's desire to try to limit the set of > supported compilers. Yes, and I agree we're long overdue with raising the baseline. It's just that it's not straightforward to establish a good new one. Jan
On 10/03/2021 10:13, Jan Beulich wrote: > Sadly I was wrong to suggest dropping vaddrs' initializer during review > of v2 of the patch introducing this code. gcc 4.3 can't cope. > > Fixes: 52531c734ea1 ("xen/gnttab: Rework resource acquisition") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -4026,7 +4026,7 @@ int gnttab_acquire_resource( > struct grant_table *gt = d->grant_table; > unsigned int i, final_frame; > mfn_t tmp; > - void **vaddrs; > + void **vaddrs = NULL; > int rc = -EINVAL; > > if ( !nr_frames ) in v1, there was a companion check. diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index f937c1d350..2bb07f129f 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -4059,6 +4059,16 @@ int gnttab_acquire_resource( if ( rc ) goto out; + /* + * Some older toolchains can't spot that vaddrs is non-NULL on non-error + * paths. Leave some runtime safety. + */ + if ( !vaddrs ) + { + ASSERT_UNREACHABLE(); + goto out; + } + for ( i = 0; i < nr_frames; ++i ) mfn_list[i] = virt_to_mfn(vaddrs[frame + i]); With this reinstated, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 12.03.2021 12:32, Andrew Cooper wrote: > On 10/03/2021 10:13, Jan Beulich wrote: >> Sadly I was wrong to suggest dropping vaddrs' initializer during review >> of v2 of the patch introducing this code. gcc 4.3 can't cope. >> >> Fixes: 52531c734ea1 ("xen/gnttab: Rework resource acquisition") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -4026,7 +4026,7 @@ int gnttab_acquire_resource( >> struct grant_table *gt = d->grant_table; >> unsigned int i, final_frame; >> mfn_t tmp; >> - void **vaddrs; >> + void **vaddrs = NULL; >> int rc = -EINVAL; >> >> if ( !nr_frames ) > > in v1, there was a companion check. > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c > index f937c1d350..2bb07f129f 100644 > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -4059,6 +4059,16 @@ int gnttab_acquire_resource( > if ( rc ) > goto out; > > + /* > + * Some older toolchains can't spot that vaddrs is non-NULL on > non-error > + * paths. Leave some runtime safety. > + */ > + if ( !vaddrs ) > + { > + ASSERT_UNREACHABLE(); > + goto out; > + } > + > for ( i = 0; i < nr_frames; ++i ) > mfn_list[i] = virt_to_mfn(vaddrs[frame + i]); Oh, I didn't realize this. Will add, but did you really mean to have the function return success in this case (on a release build)? I'd be inclined to put it ahead of if "if ( rc )" and set rc (to e.g. -ENODATA) in this case. > With this reinstated, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. Jan
On 12.03.2021 14:08, Jan Beulich wrote: > On 12.03.2021 12:32, Andrew Cooper wrote: >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -4059,6 +4059,16 @@ int gnttab_acquire_resource( >> if ( rc ) >> goto out; >> >> + /* >> + * Some older toolchains can't spot that vaddrs is non-NULL on >> non-error >> + * paths. Leave some runtime safety. >> + */ >> + if ( !vaddrs ) >> + { >> + ASSERT_UNREACHABLE(); >> + goto out; >> + } >> + >> for ( i = 0; i < nr_frames; ++i ) >> mfn_list[i] = virt_to_mfn(vaddrs[frame + i]); > > Oh, I didn't realize this. Will add, but did you really mean to > have the function return success in this case (on a release > build)? I'd be inclined to put it ahead of if "if ( rc )" and > set rc (to e.g. -ENODATA) in this case. But I think the comment isn't really correct - the problem isn't NULL or not, but uninitialized without setting it to NULL. How about /* * Some older toolchains can't spot that vaddrs won't remain uninitialized * on non-error paths, and hence it needs setting to NULL at the top of the * function. Leave some runtime safety. */ ? Jan
On 12/03/2021 13:18, Jan Beulich wrote: > On 12.03.2021 14:08, Jan Beulich wrote: >> On 12.03.2021 12:32, Andrew Cooper wrote: >>> --- a/xen/common/grant_table.c >>> +++ b/xen/common/grant_table.c >>> @@ -4059,6 +4059,16 @@ int gnttab_acquire_resource( >>> if ( rc ) >>> goto out; >>> >>> + /* >>> + * Some older toolchains can't spot that vaddrs is non-NULL on >>> non-error >>> + * paths. Leave some runtime safety. >>> + */ >>> + if ( !vaddrs ) >>> + { >>> + ASSERT_UNREACHABLE(); >>> + goto out; >>> + } >>> + >>> for ( i = 0; i < nr_frames; ++i ) >>> mfn_list[i] = virt_to_mfn(vaddrs[frame + i]); >> Oh, I didn't realize this. Will add, but did you really mean to >> have the function return success in this case (on a release >> build)? I'd be inclined to put it ahead of if "if ( rc )" and >> set rc (to e.g. -ENODATA) in this case. > But I think the comment isn't really correct - the problem isn't > NULL or not, but uninitialized without setting it to NULL. How > about > > /* > * Some older toolchains can't spot that vaddrs won't remain uninitialized > * on non-error paths, and hence it needs setting to NULL at the top of the > * function. Leave some runtime safety. > */ > > ? Yes - that's fine. ~Andrew
On 12/03/2021 13:08, Jan Beulich wrote: > On 12.03.2021 12:32, Andrew Cooper wrote: >> On 10/03/2021 10:13, Jan Beulich wrote: >>> Sadly I was wrong to suggest dropping vaddrs' initializer during review >>> of v2 of the patch introducing this code. gcc 4.3 can't cope. >>> >>> Fixes: 52531c734ea1 ("xen/gnttab: Rework resource acquisition") >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> --- a/xen/common/grant_table.c >>> +++ b/xen/common/grant_table.c >>> @@ -4026,7 +4026,7 @@ int gnttab_acquire_resource( >>> struct grant_table *gt = d->grant_table; >>> unsigned int i, final_frame; >>> mfn_t tmp; >>> - void **vaddrs; >>> + void **vaddrs = NULL; >>> int rc = -EINVAL; >>> >>> if ( !nr_frames ) >> in v1, there was a companion check. >> >> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c >> index f937c1d350..2bb07f129f 100644 >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -4059,6 +4059,16 @@ int gnttab_acquire_resource( >> if ( rc ) >> goto out; >> >> + /* >> + * Some older toolchains can't spot that vaddrs is non-NULL on >> non-error >> + * paths. Leave some runtime safety. >> + */ >> + if ( !vaddrs ) >> + { >> + ASSERT_UNREACHABLE(); >> + goto out; >> + } >> + >> for ( i = 0; i < nr_frames; ++i ) >> mfn_list[i] = virt_to_mfn(vaddrs[frame + i]); > Oh, I didn't realize this. Will add, but did you really mean to > have the function return success in this case (on a release > build)? I'd be inclined to put it ahead of if "if ( rc )" and > set rc (to e.g. -ENODATA) in this case. Oh - quite right. Returning 0 here will hit the assertion/failsafe protecting against livelock. I'd be tempted to chose -EINVAL because the only plausible way to get here is a bad id, and that path should have errored out earlier. And yeah, with the rc adjustment, fine to reposition. ~Andrew
On 12.03.2021 14:25, Andrew Cooper wrote: > On 12/03/2021 13:08, Jan Beulich wrote: >> On 12.03.2021 12:32, Andrew Cooper wrote: >>> On 10/03/2021 10:13, Jan Beulich wrote: >>>> Sadly I was wrong to suggest dropping vaddrs' initializer during review >>>> of v2 of the patch introducing this code. gcc 4.3 can't cope. >>>> >>>> Fixes: 52531c734ea1 ("xen/gnttab: Rework resource acquisition") >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> >>>> --- a/xen/common/grant_table.c >>>> +++ b/xen/common/grant_table.c >>>> @@ -4026,7 +4026,7 @@ int gnttab_acquire_resource( >>>> struct grant_table *gt = d->grant_table; >>>> unsigned int i, final_frame; >>>> mfn_t tmp; >>>> - void **vaddrs; >>>> + void **vaddrs = NULL; >>>> int rc = -EINVAL; >>>> >>>> if ( !nr_frames ) >>> in v1, there was a companion check. >>> >>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c >>> index f937c1d350..2bb07f129f 100644 >>> --- a/xen/common/grant_table.c >>> +++ b/xen/common/grant_table.c >>> @@ -4059,6 +4059,16 @@ int gnttab_acquire_resource( >>> if ( rc ) >>> goto out; >>> >>> + /* >>> + * Some older toolchains can't spot that vaddrs is non-NULL on >>> non-error >>> + * paths. Leave some runtime safety. >>> + */ >>> + if ( !vaddrs ) >>> + { >>> + ASSERT_UNREACHABLE(); >>> + goto out; >>> + } >>> + >>> for ( i = 0; i < nr_frames; ++i ) >>> mfn_list[i] = virt_to_mfn(vaddrs[frame + i]); >> Oh, I didn't realize this. Will add, but did you really mean to >> have the function return success in this case (on a release >> build)? I'd be inclined to put it ahead of if "if ( rc )" and >> set rc (to e.g. -ENODATA) in this case. > > Oh - quite right. Returning 0 here will hit the assertion/failsafe > protecting against livelock. > > I'd be tempted to chose -EINVAL because the only plausible way to get > here is a bad id, and that path should have errored out earlier. As you may have seen, I've chosen to stick to ENODATA. This error, should it ever get raised, would better be easily distinguishable from an ordinary -EINVAL. Jan
On 12.03.2021 11:04, Ian Jackson wrote: > Julien Grall writes ("Re: [PATCH][4.15] gnttab: work around "may be used uninitialized" warning"): >> This is pretty much what we are already doing slowly by initializing >> values to shut up older compilers. I agree this is more limited, but we >> also waive off diagnostics from every single compiler in that code >> rather than just one version. >> >> Hence why I suggested dropping -Werror for older compiler. This is not >> ideal but it would give us the ability to keep support for dinausor >> compiler and not hamper our ability to take advantage of newer compiler >> diagnostics. > > I agree with Julien. I think we should avoid adding these redundant > initialisers for the reasons he gives. > >> The ideal solution is to drop support for older compiler (see my other >> thread). But this sounds like a daunting task so far on x86... >> >> Anyway, I will not Nack the patch but will also not Ack it. I will let >> another maintainer ack this patch. > > But this is outside my usual area so I won't nack it either. But would you be willing to release-ack v2? Jan
Jan Beulich writes ("Re: [PATCH][4.15] gnttab: work around "may be used uninitialized" warning"):
> On 12.03.2021 11:04, Ian Jackson wrote:
> > But this is outside my usual area so I won't nack it either.
>
> But would you be willing to release-ack v2?
Good question. I don't think my code quality/style qualms etc. have
any bearing on the release question.
So, I will do that now:
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
Ian.
--- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -4026,7 +4026,7 @@ int gnttab_acquire_resource( struct grant_table *gt = d->grant_table; unsigned int i, final_frame; mfn_t tmp; - void **vaddrs; + void **vaddrs = NULL; int rc = -EINVAL; if ( !nr_frames )
Sadly I was wrong to suggest dropping vaddrs' initializer during review of v2 of the patch introducing this code. gcc 4.3 can't cope. Fixes: 52531c734ea1 ("xen/gnttab: Rework resource acquisition") Signed-off-by: Jan Beulich <jbeulich@suse.com>