[RFC,v5,01/21] sunrpc/clnt_udp: Ensure total_deadline is initalised

  Even though total_deadline won't be accessed uninitalised GCC can still
complain that it is accessed unitalised, to avod those errors let's make
sure we initalise it to 0.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

	* sunrpc/clnt_udp.c: Ensure total_deadline is initalised.
---
 sunrpc/clnt_udp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On Thu, 29 Aug 2019, Alistair Francis wrote:

> Even though total_deadline won't be accessed uninitalised GCC can still
> complain that it is accessed unitalised, to avod those errors let's make
> sure we initalise it to 0.

It's glibc practice (although missing from 
<https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't* 
add initializations like that to avoid warnings.  If necessary we may use 
the DIAG_* macros with appropriate comments about why the warning is a 
false positive (sometimes other approaches work to avoid the warnings, 
e.g. use of __builtin_unreachable () calls).
  

On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote:
> On Thu, 29 Aug 2019, Alistair Francis wrote:
>The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know.  I'm assu
> > Even though total_deadline won't be accessed uninitalised GCC can still
> > complain that it is accessed unitalised, to avod those errors let's make
> > sure we initalise it to 0.
>
> It's glibc practice (although missing from
> <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't*
> add initializations like that to avoid warnings.

Although this has historically been glibc practice, I think it is
unwisely incautious, and we should change the policy to be that we
*do* add initializations whenever the compiler thinks a variable even
_might_ be used uninitialized.

zw
  

On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote:
>
> On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote:
> > On Thu, 29 Aug 2019, Alistair Francis wrote:
> >The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know.  I'm assu
> > > Even though total_deadline won't be accessed uninitalised GCC can still
> > > complain that it is accessed unitalised, to avod those errors let's make
> > > sure we initalise it to 0.
> >
> > It's glibc practice (although missing from
> > <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't*
> > add initializations like that to avoid warnings.
>
> Although this has historically been glibc practice, I think it is
> unwisely incautious, and we should change the policy to be that we
> *do* add initializations whenever the compiler thinks a variable even
> _might_ be used uninitialized.

Does that mean this patch is ok?

Alistair

>
> zw
  

On Thu, 5 Sep 2019, Alistair Francis wrote:

> On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote:
> >
> > On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote:
> > > On Thu, 29 Aug 2019, Alistair Francis wrote:
> > >The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know.  I'm assu
> > > > Even though total_deadline won't be accessed uninitalised GCC can still
> > > > complain that it is accessed unitalised, to avod those errors let's make
> > > > sure we initalise it to 0.
> > >
> > > It's glibc practice (although missing from
> > > <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't*
> > > add initializations like that to avoid warnings.
> >
> > Although this has historically been glibc practice, I think it is
> > unwisely incautious, and we should change the policy to be that we
> > *do* add initializations whenever the compiler thinks a variable even
> > _might_ be used uninitialized.
> 
> Does that mean this patch is ok?

No.  You can't deduce consensus like that from two different views on a 
patch or a convention.  Even if we were to change the convention regarding 
how to silence such warnings, I see reason to have any less requirement 
for comments explaining why the warning is a false positive and that the 
initializer is only there to silence a warning than there is for the 
DIAG_* macros.
  

On 9/5/19 9:02 AM, Joseph Myers wrote:
> On Thu, 5 Sep 2019, Alistair Francis wrote:
> 
>> On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote:
>>>
>>> On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote:
>>>> On Thu, 29 Aug 2019, Alistair Francis wrote:
>>>> The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know.  I'm assu
>>>>> Even though total_deadline won't be accessed uninitalised GCC can still
>>>>> complain that it is accessed unitalised, to avod those errors let's make
>>>>> sure we initalise it to 0.
>>>>
>>>> It's glibc practice (although missing from
>>>> <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't*
>>>> add initializations like that to avoid warnings.
>>>
>>> Although this has historically been glibc practice, I think it is
>>> unwisely incautious, and we should change the policy to be that we
>>> *do* add initializations whenever the compiler thinks a variable even
>>> _might_ be used uninitialized.
>>
>> Does that mean this patch is ok?
> 
> No.  You can't deduce consensus like that from two different views on a 
> patch or a convention.  Even if we were to change the convention regarding 
> how to silence such warnings, I see reason to have any less requirement 
> for comments explaining why the warning is a false positive and that the 
> initializer is only there to silence a warning than there is for the 
> DIAG_* macros.
> 
BTW, has a bug been filed against GCC for the bogus warning?

Jeff
  

On Thu, Sep 5, 2019 at 10:53 AM Alistair Francis <alistair23@gmail.com> wrote:
> On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote:
> > On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote:
> > > On Thu, 29 Aug 2019, Alistair Francis wrote:
> > >The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know.  I'm assu
> > > > Even though total_deadline won't be accessed uninitalised GCC can still
> > > > complain that it is accessed unitalised, to avod those errors let's make
> > > > sure we initalise it to 0.
> > >
> > > It's glibc practice (although missing from
> > > <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't*
> > > add initializations like that to avoid warnings.
> >
> > Although this has historically been glibc practice, I think it is
> > unwisely incautious, and we should change the policy to be that we
> > *do* add initializations whenever the compiler thinks a variable even
> > _might_ be used uninitialized.
>
> Does that mean this patch is ok?

I haven't actually looked at your patch, so no, that was not a review.

zw
  

On Thu, Sep 5, 2019 at 11:03 AM Joseph Myers <joseph@codesourcery.com> wrote:
> On Thu, 5 Sep 2019, Alistair Francis wrote:
> > On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote:
> > >
> > > On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote:
> > > > On Thu, 29 Aug 2019, Alistair Francis wrote:
> > > > > Even though total_deadline won't be accessed uninitalised GCC can still
> > > > > complain that it is accessed unitalised, to avod those errors let's make
> > > > > sure we initalise it to 0.
> > > >
> > > > It's glibc practice (although missing from
> > > > <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't*
> > > > add initializations like that to avoid warnings.
> > >
> > > Although this has historically been glibc practice, I think it is
> > > unwisely incautious, and we should change the policy to be that we
> > > *do* add initializations whenever the compiler thinks a variable even
> > > _might_ be used uninitialized.
> >
> > Does that mean this patch is ok?
>
> No.  You can't deduce consensus like that from two different views on a
> patch or a convention.

Joseph, you have a tendency to throw up procedural objections to
_everything_, and I feel I need to ask you to tone it down a little.
This project has historically been perceived as unwelcoming to new
contributors, and I think a major remaining reason for that is all the
procedure -- some of which is genuinely necessary, but not all.

> Even if we were to change the convention regarding
> how to silence such warnings, I see reason to have any less requirement
> for comments explaining why the warning is a false positive and that the
> initializer is only there to silence a warning than there is for the
> DIAG_* macros.

In general I would agree, but this is the obsolete sunrpc directory
we're talking about; I don't think it's worth putting more than the
bare minimum effort into code that we plan to delete eventually.

zw
  

On Thu, 5 Sep 2019, Jeff Law wrote:

> BTW, has a bug been filed against GCC for the bogus warning?

In this case, it looks like the code is

  if (xargs != NULL)
    {
       total_deadline = ...
    }

  ...

  if (xargs != NULL)
    {
      use total_deadline
    }

(and xargs doesn't get modified in the function and the various gotos in 
this function are all after that setting of total_deadline).  So that 
should be a known issue (and we could consider if the existing comment

  /* Choose the timeout value.  For non-sending usage (xargs == NULL),
     the total deadline does not matter, only cu->cu_wait is used
     below.  */

is sufficient or should be extended to say explicitly the warning is 
bogus).
  

On 9/5/19 9:21 AM, Joseph Myers wrote:
> On Thu, 5 Sep 2019, Jeff Law wrote:
> 
>> BTW, has a bug been filed against GCC for the bogus warning?
> 
> In this case, it looks like the code is
> 
>   if (xargs != NULL)
>     {
>        total_deadline = ...
>     }
> 
>   ...
> 
>   if (xargs != NULL)
>     {
>       use total_deadline
>     }
> 
> (and xargs doesn't get modified in the function and the various gotos in 
> this function are all after that setting of total_deadline).  So that 
> should be a known issue (and we could consider if the existing comment
> 
>   /* Choose the timeout value.  For non-sending usage (xargs == NULL),
>      the total deadline does not matter, only cu->cu_wait is used
>      below.  */
> 
> is sufficient or should be extended to say explicitly the warning is 
> bogus).
Right.  My concern is we should be analyzing this on the GCC side.

I can hazard a guess that there's too much code between the two tests
and as a result jump threading isn't deemed profitable enough.  That's
not terribly unusual.

But even so, the uninit pass has predicate analysis which should see the
use as properly guarded.

So ISTM that something isn't behaving properly on the GCC side so we
should get bug report opened so that we can analyze why GCC isn't doing
the right thing here.

jeff
  

On Thu, Sep 5, 2019 at 8:02 AM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Thu, 5 Sep 2019, Alistair Francis wrote:
>
> > On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote:
> > >
> > > On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote:
> > > > On Thu, 29 Aug 2019, Alistair Francis wrote:
> > > >The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know.  I'm assu
> > > > > Even though total_deadline won't be accessed uninitalised GCC can still
> > > > > complain that it is accessed unitalised, to avod those errors let's make
> > > > > sure we initalise it to 0.
> > > >
> > > > It's glibc practice (although missing from
> > > > <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't*
> > > > add initializations like that to avoid warnings.
> > >
> > > Although this has historically been glibc practice, I think it is
> > > unwisely incautious, and we should change the policy to be that we
> > > *do* add initializations whenever the compiler thinks a variable even
> > > _might_ be used uninitialized.
> >
> > Does that mean this patch is ok?
>
> No.  You can't deduce consensus like that from two different views on a
> patch or a convention.  Even if we were to change the convention regarding
> how to silence such warnings, I see reason to have any less requirement
> for comments explaining why the warning is a false positive and that the
> initializer is only there to silence a warning than there is for the
> DIAG_* macros.

No worries, I'll happily change the patch, I just want to make sure I
change it to the right thing.

I'll:
 - Investigate filing a GCC bug for this false positive
 - Add comments to the init explaining why I am setting it
 - Use the DIAG_* macros

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
  

On Thu, Sep 5, 2019 at 8:22 AM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Thu, 5 Sep 2019, Jeff Law wrote:
>
> > BTW, has a bug been filed against GCC for the bogus warning?
>
> In this case, it looks like the code is
>
>   if (xargs != NULL)
>     {
>        total_deadline = ...
>     }
>
>   ...
>
>   if (xargs != NULL)
>     {
>       use total_deadline
>     }
>
> (and xargs doesn't get modified in the function and the various gotos in
> this function are all after that setting of total_deadline).  So that
> should be a known issue (and we could consider if the existing comment

That is my understanding as well.

>
>   /* Choose the timeout value.  For non-sending usage (xargs == NULL),
>      the total deadline does not matter, only cu->cu_wait is used
>      below.  */
>
> is sufficient or should be extended to say explicitly the warning is
> bogus).

I was planning on adding a comment in the next version, let me know otherwise.

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
  

On 9/5/19 6:46 PM, Alistair Francis wrote:
> On Thu, Sep 5, 2019 at 8:02 AM Joseph Myers <joseph@codesourcery.com> wrote:
>>
>> On Thu, 5 Sep 2019, Alistair Francis wrote:
>>
>>> On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote:
>>>>
>>>> On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote:
>>>>> On Thu, 29 Aug 2019, Alistair Francis wrote:
>>>>> The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know.  I'm assu
>>>>>> Even though total_deadline won't be accessed uninitalised GCC can still
>>>>>> complain that it is accessed unitalised, to avod those errors let's make
>>>>>> sure we initalise it to 0.
>>>>>
>>>>> It's glibc practice (although missing from
>>>>> <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't*
>>>>> add initializations like that to avoid warnings.
>>>>
>>>> Although this has historically been glibc practice, I think it is
>>>> unwisely incautious, and we should change the policy to be that we
>>>> *do* add initializations whenever the compiler thinks a variable even
>>>> _might_ be used uninitialized.
>>>
>>> Does that mean this patch is ok?
>>
>> No.  You can't deduce consensus like that from two different views on a
>> patch or a convention.  Even if we were to change the convention regarding
>> how to silence such warnings, I see reason to have any less requirement
>> for comments explaining why the warning is a false positive and that the
>> initializer is only there to silence a warning than there is for the
>> DIAG_* macros.
> 
> No worries, I'll happily change the patch, I just want to make sure I
> change it to the right thing.
> 
> I'll:
>  - Investigate filing a GCC bug for this false positive
All we need is the .i file (Add -save-temps to the compilation line), a
copy of the full compilation line and the target triplet.  No need to
try and create a minimal testcase or anything like that.

Jeff
  

On Thu, Sep 5, 2019 at 6:31 PM Jeff Law <law@redhat.com> wrote:
>
> On 9/5/19 6:46 PM, Alistair Francis wrote:
> > On Thu, Sep 5, 2019 at 8:02 AM Joseph Myers <joseph@codesourcery.com> wrote:
> >>
> >> On Thu, 5 Sep 2019, Alistair Francis wrote:
> >>
> >>> On Thu, Aug 29, 2019 at 10:34 AM Zack Weinberg <zackw@panix.com> wrote:
> >>>>
> >>>> On Thu, Aug 29, 2019 at 1:22 PM Joseph Myers <joseph@codesourcery.com> wrote:
> >>>>> On Thu, 29 Aug 2019, Alistair Francis wrote:
> >>>>> The long pole is definitely the ml2014 build environment, unless for some reason we need the new version of pip first? I don't actually know.  I'm assu
> >>>>>> Even though total_deadline won't be accessed uninitalised GCC can still
> >>>>>> complain that it is accessed unitalised, to avod those errors let's make
> >>>>>> sure we initalise it to 0.
> >>>>>
> >>>>> It's glibc practice (although missing from
> >>>>> <https://sourceware.org/glibc/wiki/Style_and_Conventions>) that we *don't*
> >>>>> add initializations like that to avoid warnings.
> >>>>
> >>>> Although this has historically been glibc practice, I think it is
> >>>> unwisely incautious, and we should change the policy to be that we
> >>>> *do* add initializations whenever the compiler thinks a variable even
> >>>> _might_ be used uninitialized.
> >>>
> >>> Does that mean this patch is ok?
> >>
> >> No.  You can't deduce consensus like that from two different views on a
> >> patch or a convention.  Even if we were to change the convention regarding
> >> how to silence such warnings, I see reason to have any less requirement
> >> for comments explaining why the warning is a false positive and that the
> >> initializer is only there to silence a warning than there is for the
> >> DIAG_* macros.
> >
> > No worries, I'll happily change the patch, I just want to make sure I
> > change it to the right thing.
> >
> > I'll:
> >  - Investigate filing a GCC bug for this false positive
> All we need is the .i file (Add -save-temps to the compilation line), a
> copy of the full compilation line and the target triplet.  No need to
> try and create a minimal testcase or anything like that.

Thanks for that info. I have filed the bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91691

I'll send this patch out separately and we can go from there.

Alistair

>
> Jeff