Re: Possible non-compliance of the restrict keyword contract in coreutils/touch.c

[David Malcolm]

On Wed, 2022-07-27 at 21:27 +0200, Bruno Haible wrote:
> [Removing coreutils from CC.]
> Paul Eggert wrote:
> > Thanks for the bug report. It's clearly a bug in 'touch'. I
> > installed 
> > the attached patch into Coreutils master on Savannah.
> 
> It's good to see that the 'restrict' annotations that we discussed in
> February 2020 [1] and then added [2] actually caught a real bug.
> 
> > It strikes me that the signatures for parse_datetime and
> > parse_datetime2 
> > could use more 'restrict's. For example, instead of this:
> > 
> >    bool parse_datetime (struct timespec *restrict,
> > 
> >                         char const *,
> >                         struct timespec const *);
> > 
> > 
> > gnulib/lib/parse-datetime.h should have this:
> > 
> >    bool parse_datetime (struct timespec *restrict,
> > 
> >                         char const *restrict,
> >                         struct timespec const *restrict);
> > 
> > 
> > This is the usual pattern in the C standard and POSIX; see
> > strptime, for 
> > example[1]. Any reason not to make this change to Gnulib?
> 
> We had this discussion already in Feb 2020: You advocated using as
> much
> 'restrict' as possible [3][4]. It didn't convince me, and I wrote [5]
>   "The semantics of 'restrict' is a bit confusing, and the semantics
>    of 'restrict' on const pointer parameters is additionally
> confusing."
> 
> So, in my opinion, what we need is not to add more 'restrict' here
> and
> there, but rather an understandable (and still correct) explanation
> of
> what 'restrict' means, including for const pointers and array
> members.
> If stackoverflow is possibly not the right medium for this
> explanation,
> then possibly the GNU C manual [6] is? So far, it does not explain
> anything
> regarding 'restrict'.

I'm the maintainer of GCC's -fanalyzer option.  (Paul: thanks for all
the bugs you've been filing!)


FWIW the reference I've been using is:
  https://en.cppreference.com/w/c/language/restrict
though I had to stare at that, and at the code in question before
deciding that Tim's experimental warning appears to be reporting a true
positive.

The presence of the "restrict" on 1st arg of the decl of parse_datetime
could be interpreted as a warning not to pass the same timespec ptr to
both arguments 1 and 3; that the implementation is free to start
writing through the 1st ptr before it's finished reading through the
3rd ptr.

Looking at:
  https://github.com/coreutils/gnulib/blob/master/lib/parse-datetime.y#L2400
I see that parse_datetime calls parse_datetime_body.
which is lines 1709-2373 of that .y file (i.e. about 600 lines).
The final read through now-> seems to be at line 1791.  The first write
through result-> seems to be at line 2290.  So I think the code happens
to be safe, but get_reldate is doing something that parse_datetime's
decl is explicitly saying not to.  Hence I think it's fair for Tim's
new warning to fire on this code.  Further, there's a chance that GCC's
optimizers might at some point make use of the restrict marking
(perhaps with LTO), and do something to the code you weren't expecting.

Hope this makes sense and is constructive
Dave

> 
> Bruno
> 
> [1]  https://lists.gnu.org/archive/html/bug-gnulib/2020-02/threads.html
> [2] 
> https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=5f56bf12b34a83ab90c9d7e3955aacb9c67cb8a2
> [3] 
> https://lists.gnu.org/archive/html/bug-gnulib/2020-02/msg00096.html
> [4] 
> https://lists.gnu.org/archive/html/bug-gnulib/2020-02/msg00100.html
> [5] 
> https://lists.gnu.org/archive/html/bug-gnulib/2020-02/msg00099.html
> [6] https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html
> 
> 
>