Comments

At this stage I don’t think we can issue warnings for settimeofday
with a non-null tzp argument, nor for arbitrary use of struct
timezone. But we can warn about gettimeofday with non-null tzp.
This uses a macro instead of an inline (fortify-style) function
because I got false positives with an inline, even with GCC 9.
* time/sys/time.h (__timezone_ptr_t): Delete.
(gettimeofday): Always declare second argument with type ‘void *’.
When possible, wrap with a macro that detects non-null and
non-constant second argument and issues a warning.
Improve commentary.
(settimeofday): Improve commentary.
* time/gettimeofday.c (gettimeofday):
Declare second argument as type ‘void *’.
---
time/gettimeofday.c | 4 ++--
time/sys/time.h | 36 +++++++++++++++++++++++++-----------
2 files changed, 27 insertions(+), 13 deletions(-)

On Wed, Aug 21, 2019 at 11:30 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> On 20/08/2019 10:21, Zack Weinberg wrote:> > At this stage I don’t think we can issue warnings for settimeofday> > with a non-null tzp argument, nor for arbitrary use of struct> > timezone. But we can warn about gettimeofday with non-null tzp.> >> > This uses a macro instead of an inline (fortify-style) function> > because I got false positives with an inline, even with GCC 9.>> Would be troublesome to add a check for the new warning?
Do we have any infrastructure for testing for warnings? I don't know about any.
> Also is the> false positive a GCC defect or something limited with the inline> usage?
I didn't look into it in any detail, but the uses of __warndecl +
__builtin_constant_p in bits/string2.h inlines all follow the pattern
__extern_always_inline T func (ptr)
{
if (__builtin_constant_p (ptr) && something_else_p (ptr))
__issue_warning ();
...
}
that is, issue a warning if ptr is a compile-time constant with some
property. What we want for gettimeofday is just the opposite,
if (! (__builtin_constant_p (ptr) && ptr == NULL)) __issue_warning ();
that is, warn whenever ptr is _not_ a compile-time NULL. I wouldn't
be surprised if GCC's earliest unreachable-code removal passes, the
ones that happen early enough to prevent __attribute__((warning))
diagnostics from triggering, were tuned precisely for what
bits/string2.h does, and don't handle this inverse case properly.
zw

* Zack Weinberg:
> I didn't look into it in any detail, but the uses of __warndecl +> __builtin_constant_p in bits/string2.h inlines all follow the pattern>> __extern_always_inline T func (ptr)> {> if (__builtin_constant_p (ptr) && something_else_p (ptr))> __issue_warning ();> ...> }>> that is, issue a warning if ptr is a compile-time constant with some> property. What we want for gettimeofday is just the opposite,>> if (! (__builtin_constant_p (ptr) && ptr == NULL)) __issue_warning ();>> that is, warn whenever ptr is _not_ a compile-time NULL. I wouldn't> be surprised if GCC's earliest unreachable-code removal passes, the> ones that happen early enough to prevent __attribute__((warning))> diagnostics from triggering, were tuned precisely for what> bits/string2.h does, and don't handle this inverse case properly.
I don't think this will work. You would have to use something like
this:
__builtin_constant_p (ptr != NULL) && ptr != NULL
Otherwise you will produce a warning every time someone uses the
gettimeofday wrapper in a function for which optimization has been
disabled.
Thanks,
Florian

On 8/21/19 12:10 PM, Florian Weimer wrote:
> * Zack Weinberg:> >> I didn't look into it in any detail, but the uses of __warndecl +>> __builtin_constant_p in bits/string2.h inlines all follow the pattern>>>> __extern_always_inline T func (ptr)>> {>> if (__builtin_constant_p (ptr) && something_else_p (ptr))>> __issue_warning ();>> ...>> }>>>> that is, issue a warning if ptr is a compile-time constant with some>> property. What we want for gettimeofday is just the opposite,>>>> if (! (__builtin_constant_p (ptr) && ptr == NULL)) __issue_warning ();>>>> that is, warn whenever ptr is _not_ a compile-time NULL. I wouldn't>> be surprised if GCC's earliest unreachable-code removal passes, the>> ones that happen early enough to prevent __attribute__((warning))>> diagnostics from triggering, were tuned precisely for what>> bits/string2.h does, and don't handle this inverse case properly.> > I don't think this will work. You would have to use something like> this:> > __builtin_constant_p (ptr != NULL) && ptr != NULL> > Otherwise you will produce a warning every time someone uses the> gettimeofday wrapper in a function for which optimization has been> disabled.
That is the behavior I was seeing when the wrapper was an extern inline,
but not when it is a macro. I'm going to do more thorough testing.
Also, `__builtin_constant_p (ptr != NULL) && ptr != NULL` would warn
only for compile-time non-NULL, if I'm reading it right. But in this
case we also want to issue a warning for any argument that isn't a
compile-time constant. In other words, we want to warn for everything
_except_ compile-time NULL. Hence `! (__builtin_constant_p (ptr) && ptr
== NULL)`.
zw

* Zack Weinberg:
> On 8/21/19 12:10 PM, Florian Weimer wrote:>> * Zack Weinberg:>> >>> I didn't look into it in any detail, but the uses of __warndecl +>>> __builtin_constant_p in bits/string2.h inlines all follow the pattern>>>>>> __extern_always_inline T func (ptr)>>> {>>> if (__builtin_constant_p (ptr) && something_else_p (ptr))>>> __issue_warning ();>>> ...>>> }>>>>>> that is, issue a warning if ptr is a compile-time constant with some>>> property. What we want for gettimeofday is just the opposite,>>>>>> if (! (__builtin_constant_p (ptr) && ptr == NULL)) __issue_warning ();>>>>>> that is, warn whenever ptr is _not_ a compile-time NULL. I wouldn't>>> be surprised if GCC's earliest unreachable-code removal passes, the>>> ones that happen early enough to prevent __attribute__((warning))>>> diagnostics from triggering, were tuned precisely for what>>> bits/string2.h does, and don't handle this inverse case properly.>> >> I don't think this will work. You would have to use something like>> this:>> >> __builtin_constant_p (ptr != NULL) && ptr != NULL>> >> Otherwise you will produce a warning every time someone uses the>> gettimeofday wrapper in a function for which optimization has been>> disabled.>> That is the behavior I was seeing when the wrapper was an extern inline,> but not when it is a macro. I'm going to do more thorough testing.
I don't think we should turn gettimeofday into a macro. It's likely to
cause problems with some C++ wrapper libraries.
> Also, `__builtin_constant_p (ptr != NULL) && ptr != NULL` would warn> only for compile-time non-NULL, if I'm reading it right.
Correct.
> But in this case we also want to issue a warning for any argument that> isn't a compile-time constant. In other words, we want to warn for> everything _except_ compile-time NULL. Hence `! (__builtin_constant_p> (ptr) && ptr == NULL)`.
I think this is simply not possible because it will cause unpredictable
false positives.
Thanks,
Florian

Patch

diff --git a/time/gettimeofday.c b/time/gettimeofday.cindex 22a996a220..bd1fc3cb5e 100644--- a/time/gettimeofday.c+++ b/time/gettimeofday.c@@ -23,10 +23,10 @@
If *TZ is not NULL, clear it.
Returns 0 on success, -1 on errors. */
int
-__gettimeofday (struct timeval *tv, struct timezone *tz)+__gettimeofday (struct timeval *restrict tv, void *restrict tz)
{
if (__glibc_unlikely (tz != 0))
- memset (tz, 0, sizeof *tz);+ memset (tz, 0, sizeof (struct timezone));
struct timespec ts;
if (__clock_gettime (CLOCK_REALTIME, &ts))
diff --git a/time/sys/time.h b/time/sys/time.hindex 5dbc7fc627..1b6c112274 100644--- a/time/sys/time.h+++ b/time/sys/time.h@@ -54,23 +54,37 @@ struct timezone
int tz_minuteswest; /* Minutes west of GMT. */
int tz_dsttime; /* Nonzero if DST is ever in effect. */
};
--typedef struct timezone *__restrict __timezone_ptr_t;-#else-typedef void *__restrict __timezone_ptr_t;
#endif
-/* Get the current time of day and timezone information,- putting it into *TV and *TZ. If TZ is NULL, *TZ is not filled.- Returns 0 on success, -1 on errors.- NOTE: This form of timezone information is obsolete.- Use the functions and variables declared in <time.h> instead. */+/* Get the current time of day, putting it into *TV.+ If TZ is not null, *TZ must be a struct timezone, and both fields+ will be set to zero.+ Calling this function with a non-null TZ is obsolete;+ use localtime etc. instead.+ This function itself is semi-obsolete;+ most callers should use time or clock_gettime instead. */
extern int gettimeofday (struct timeval *__restrict __tv,
- __timezone_ptr_t __tz) __THROW __nonnull ((1));+ void *__restrict __tz) __THROW __nonnull ((1));++#if __GNUC_PREREQ (4,3)+/* Issue a warning for use of gettimeofday with a non-null __tz argument. */+__warndecl (__warn_gettimeofday_timezone,+ "gettimeofday with non-null or non-constant timezone parameter;"+ " this is obsolete and inaccurate, use localtime instead");++#define gettimeofday(__tv, __tz) \+ (((!__builtin_constant_p (__tz) || (__tz) != 0) \+ ? __warn_gettimeofday_timezone () \+ : (void) 0), \+ (gettimeofday) (__tv, __tz))+#endif
#ifdef __USE_MISC
/* Set the current time of day and timezone information.
- This call is restricted to the super-user. */+ This call is restricted to the super-user.+ Setting the timezone in this way is obsolete, but we don't yet+ warn about it because it still has some uses for which there is+ no alternative. */
extern int settimeofday (const struct timeval *__tv,
const struct timezone *__tz)
__THROW;