Re: scratch_buffer and 'void *' pointer arithmetic

[Bruno Haible]

I wrote:
> struct scratch_buffer {
>   void *data;/* Pointer to the beginning of the scratch area.  */
>   size_t length; /* Allocated space at the data pointer, in bytes.  */
>   ...
> };
> 
> it seems quite natural to do pointer arithmetic on 'data'. Namely, this will
> happen each time a scratch_buffer gets booked by several (not just one)
> pieces of memory.

Oops. scratch_buffer is not appropriate for this kind of use, as it does
not contain a 'size_t used;' field (like linebuffer does).

I was thinking at a combination between scratch_buffer and the 'alloca_used'
logic from glob.c. But this is beyond what scratch_buffer does.

Bruno

Re: scratch_buffer and 'void *' pointer arithmetic

[Paul Eggert]

On 12/15/2017 07:12 PM, Bruno Haible wrote:

Therefore I would suggest to change the type of the 'data' field to 'char *'
in gnulib. I know that in glibc this is not needed, because glibc assumes GCC
and GCC supports 'void *' arithmetic as an extension.

Rationale: 'scratch_buffer' is a module of its own right in gnulib, and
may get more uses, in order to avoid stack overflows.


I think I'd rather leave it alone. glibc is moving to a more-general 
module that addresses this problem in a better way, and I'd rather we 
spend our limited resources doing that than forking.

Re: scratch_buffer and 'void *' pointer arithmetic

[Bruno Haible]

Paul Eggert wrote:
> On 12/13/2017 01:38 AM, Tim Rühsen wrote:
> > Here is a patch to silence this warning:
> >
> > glob.c: In function 'rpl_glob':
> > glob.c:618:64: warning: pointer of type 'void *' used in arithmetic
> > [-Wpointer-arith]
> > err = getpwnam_r (s.data, &pwbuf, s.data + ssize,
> >
> >
> > With Best Regards, Tim
> >
> Thanks, I tweaked it slightly to avoid inserting a cast (as it's better 
> to avoid casts when it's easy), and installed the attached patch into 
> Gnulib, in your name.

... and I've added the ChangeLog entry for it.

But, when we look at the definition

struct scratch_buffer {
  void *data;/* Pointer to the beginning of the scratch area.  */
  size_t length; /* Allocated space at the data pointer, in bytes.  */
  ...
};

it seems quite natural to do pointer arithmetic on 'data'. Namely, this will
happen each time a scratch_buffer gets booked by several (not just one)
pieces of memory.

Therefore I would suggest to change the type of the 'data' field to 'char *'
in gnulib. I know that in glibc this is not needed, because glibc assumes GCC
and GCC supports 'void *' arithmetic as an extension.

Rationale: 'scratch_buffer' is a module of its own right in gnulib, and
may get more uses, in order to avoid stack overflows.

Bruno

Re: Unneeded compiler warning in glob.c

[Paul Eggert]

On 12/13/2017 01:38 AM, Tim Rühsen wrote:

Here is a patch to silence this warning:

glob.c: In function 'rpl_glob':
glob.c:618:64: warning: pointer of type 'void *' used in arithmetic
[-Wpointer-arith]
err = getpwnam_r (s.data, &pwbuf, s.data + ssize,


With Best Regards, Tim

Thanks, I tweaked it slightly to avoid inserting a cast (as it's better 
to avoid casts when it's easy), and installed the attached patch into 
Gnulib, in your name. I'll CC: Adhemerval as this should go into glibc 
too, at some point.


>From 68e3914459c44e8cb2fed2a78840bd03c1b35084 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20R=C3=BChsen?= 
Date: Fri, 15 Dec 2017 17:27:21 -0800
Subject: [PATCH] glob.c: Silence warning about void pointer arithmetic

---
 lib/glob.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/glob.c b/lib/glob.c
index 511be12dd..ee2f9be13 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -615,7 +615,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
 {
 # if defined HAVE_GETPWNAM_R || defined _LIBC
   size_t ssize = strlen (s.data) + 1;
-  err = getpwnam_r (s.data, &pwbuf, s.data + ssize,
+  char *sdata = s.data;
+  err = getpwnam_r (sdata, &pwbuf, sdata + ssize,
 s.length - ssize, &p);
 # else
   p = getpwnam (s.data);
-- 
2.14.3

Re: latest gcc vs lib/timespec.h:85

[Paul Eggert]

On 12/14/2017 12:25 AM, Tim Rühsen wrote:

Does it fix things to add -Wshorten-64-to-32 to build-aux/gcc-warning.spec and to 
build-aux/g++-warning.spec?  > No, it doesn't change anything (I am not using 
manywarnings.m4).
OK, in that case you should be able to fix the problem by specifying 
-Wno-shorten-64-to-32 in addition to whatever other flags that you are 
specifying by hand.

Re: Warning in spawn-pipe.c (create_pipe)

[Paul Eggert]

On 12/15/2017 03:17 AM, Tim Rühsen wrote:
 >>> >>> # define NOWARN_PUSH(a) \ >>> _Pragma( STRINGIFY( clang 
diagnostic push ) ) \ >>> _Pragma( STRINGIFY( clang diagnostic ignored a 
) )
I would rather not clutter the code with calls to macros like these, as 
such calls detract from ordinary development. It's OK to put this stuff 
in a place that we ordinarily don't have to look at, but it's not OK to 
add needless complexity to code that people need to read to understand 
what the code is doing.


It ought to be easy to disable the warnings entirely, for the entire 
build, and that is better than to waste peoples' time inserting these 
calls and then later having to read them. If it's easy to do disable 
warnings on a per-module basis then I guess that's OK too. But it's not 
OK to clutter the actual code. We don't have enough resources to waste 
them on pacifying compilers that cry wolf so often.

Re: Warning in spawn-pipe.c (create_pipe)

[Tim Rühsen]

On 12/15/2017 12:54 PM, Gisle Vanem wrote:
> Tim Rühsen wrote:
> 
>> Attached is a quick commit with a new lib/pragmas.h. Sorry, I currently
>> don't have time for anything more (e.g. a module).
> 
> These are nice. MSVC also has the '__pragma()' keyword
> documented here:
>   https://msdn.microsoft.com/en-us/library/d9x1s805.aspx
> 
> But that would need a full set of 'NOWARN_xx(a)'
> for all warnings needed. E.g.
>  __pragma(warning(disable: 4101)) for 'unreferenced local variable'
> 

We would need MSVC variants of NOWARN and NOWARN_PUSH, e.g.
NOWARN_MSVC(4705 4706) or NOWARN_PUSH_MSVC(4705 4706).

Maybe you could add a tested version ?

Regards, Tim



signature.asc
Description: OpenPGP digital signature

Re: Warning in spawn-pipe.c (create_pipe)

[Gisle Vanem]

Tim Rühsen wrote:


Attached is a quick commit with a new lib/pragmas.h. Sorry, I currently
don't have time for anything more (e.g. a module).


These are nice. MSVC also has the '__pragma()' keyword
documented here:
  https://msdn.microsoft.com/en-us/library/d9x1s805.aspx

But that would need a full set of 'NOWARN_xx(a)'
for all warnings needed. E.g.
 __pragma(warning(disable: 4101)) for 'unreferenced local variable'

--
--gv

Re: Warning in spawn-pipe.c (create_pipe)

[Tim Rühsen]

On 12/15/2017 10:49 AM, Bruno Haible wrote:
> Tim Rühsen wrote:
>> It works, but... ;-)
> 
> OK, I've pushed it.
> 
>> Your patch disables that warning for the whole file. IMO, we should keep
>> the scope of the #pragma as narrow as possible.
>>
>> We could have a pragma.h include file with the following C99 code (I
>> leave the clang and gcc version checks to you):
>>
>> #define STRINGIFY(a) #a
>>
>> #if defined __clang__
>> # define NOWARN(a) _Pragma( STRINGIFY( clang diagnostic ignored a ) )
>> # define NOWARN_PUSH(a) \
>>   _Pragma( STRINGIFY( clang diagnostic push ) ) \
>>   _Pragma( STRINGIFY( clang diagnostic ignored a ) )
>> # define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )
>> #elif defined __GNUC__
>> # define NOWARN(a) _Pragma( STRINGIFY( gcc diagnostic ignored a ) )
>> # define NOWARN_PUSH(a) \
>>   _Pragma( STRINGIFY( gcc diagnostic push ) ) \
>>   _Pragma( STRINGIFY( gcc diagnostic ignored a ) )
>> # define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )
>> #else
>> # define NOWARN
>> # define NOWARN_PUSH
>> # define NOWARN_POP
>> #endif
> 
> Indeed such macrology belongs in one single file. Would you like to provide
> a patch?

Attached is a quick commit with a new lib/pragmas.h. Sorry, I currently
don't have time for anything more (e.g. a module).

> The macros should cater for the case the gcc and clang have different warning
> options (see e.g. anytostr.c).

Added a pragma to silence clang on unknown options. gcc doesn't complain
anyways. So if we need two different options, just use both.

> Also if NOWARN and NOWARN_PUSH take an argument in one case, they need to take
> an argument in the #else case as well.

Fixed.

Regards, Tim
From 63a2a7d9bf8d3a5c479dea400ca334720a30f32b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20R=C3=BChsen?= 
Date: Fri, 15 Dec 2017 11:32:32 +0100
Subject: [PATCH] lib/pragmas.h: Define for controlling compiler warning
 options

---
 lib/pragmas.h | 54 ++
 1 file changed, 54 insertions(+)
 create mode 100644 lib/pragmas.h

diff --git a/lib/pragmas.h b/lib/pragmas.h
new file mode 100644
index 0..459245dd1
--- /dev/null
+++ b/lib/pragmas.h
@@ -0,0 +1,54 @@
+/* Defines for controlling compiler warnings
+
+   Copyright (C) 2017 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see .  */
+
+#ifndef PRAGMAS_H_
+# define PRAGMAS_H_ 1
+
+/* file local stringify, undef'ed after use */
+#define GL_STRINGIFY(a) #a
+
+#if defined __clang__
+
+# define NOWARN(a) _Pragma( GL_STRINGIFY( clang diagnostic ignored a ) )
+# define NOWARN_PUSH(a) \
+  _Pragma( GL_STRINGIFY( clang diagnostic push ) ) \
+  _Pragma( GL_STRINGIFY( clang diagnostic ignored a ) )
+# define NOWARN_POP _Pragma( GL_STRINGIFY( gcc diagnostic pop ) )
+
+  /* clang complains about unknown options in diagnostic pragmas,
+ gcc doesn't. */
+  NOWARN("-Wunknown-warning-option")
+
+#elif defined __GNUC__ && ( __GNUC__ > 4 || ( __GNUC__ == 4 && __GNUC_MINOR__ >= 5 ) )
+
+# define NOWARN(a) _Pragma( GL_STRINGIFY( gcc diagnostic ignored a ) )
+# define NOWARN_PUSH(a) \
+  _Pragma( GL_STRINGIFY( gcc diagnostic push ) ) \
+  _Pragma( GL_STRINGIFY( gcc diagnostic ignored a ) )
+# define NOWARN_POP _Pragma( GL_STRINGIFY( gcc diagnostic pop ) )
+
+#else
+
+# define NOWARN(a)
+# define NOWARN_PUSH(a)
+# define NOWARN_POP
+
+#endif
+
+#undef GL_STRINGIFY
+
+#endif /* PRAGMAS_H_ */
-- 
2.15.1



signature.asc
Description: OpenPGP digital signature

Re: Warning in spawn-pipe.c (create_pipe)

[Bruno Haible]

Tim Rühsen wrote:
> It works, but... ;-)

OK, I've pushed it.

> Your patch disables that warning for the whole file. IMO, we should keep
> the scope of the #pragma as narrow as possible.
> 
> We could have a pragma.h include file with the following C99 code (I
> leave the clang and gcc version checks to you):
> 
> #define STRINGIFY(a) #a
> 
> #if defined __clang__
> # define NOWARN(a) _Pragma( STRINGIFY( clang diagnostic ignored a ) )
> # define NOWARN_PUSH(a) \
>   _Pragma( STRINGIFY( clang diagnostic push ) ) \
>   _Pragma( STRINGIFY( clang diagnostic ignored a ) )
> # define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )
> #elif defined __GNUC__
> # define NOWARN(a) _Pragma( STRINGIFY( gcc diagnostic ignored a ) )
> # define NOWARN_PUSH(a) \
>   _Pragma( STRINGIFY( gcc diagnostic push ) ) \
>   _Pragma( STRINGIFY( gcc diagnostic ignored a ) )
> # define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )
> #else
> # define NOWARN
> # define NOWARN_PUSH
> # define NOWARN_POP
> #endif

Indeed such macrology belongs in one single file. Would you like to provide
a patch?

The macros should cater for the case the gcc and clang have different warning
options (see e.g. anytostr.c).

Also if NOWARN and NOWARN_PUSH take an argument in one case, they need to take
an argument in the #else case as well.

Bruno

Re: Warning in spawn-pipe.c (create_pipe)

[Tim Rühsen]

Hi Bruno,

On 12/15/2017 12:40 AM, Bruno Haible wrote:
>>> 1) It is not a goal to have absolutely no warnings with GCC or
>>>with clang. It is perfectly OK, IMO, if a compilation with "gcc -Wall"
>>>shows, say, 5 warnings in 10 files. The maintainer will get used to
>>>these warnings and see new warnings when they arise.
>>
>> That is really bad and it makes me sad. You are saying it's a good thing
>> to get used to a bad situation. I hope you don't mean it.
> 
> Sorry but why do you call it "really bad", given that these warnings
>   - are so few that the maintainer is not hindered in their development,
>   - we know that these warnings are false positives?

- people who don't regularly build the code will get upset/alarmed by
those warnings, either thinking the maintainer is ignorant or/and even
trying to find out what's going on (wasting precious time).

- this might even repel possible contributors, especially when they
invested time, created patches and the upstream answer is "haha, sorry,
we don't want to fix these warnings - we got used to them".

- the situation might lead to switching off gnulib warnings completely
in projects that use gnulib. This means one 'line of defense' less, and
less possible contributors.

- when projects use static analysis tools and there is too much wave
from gnulib, the gnulib will simply be excluded from analysis.

If the maintainer of e.g. libz got used to some warnings... well, not so
many people/devs are affected. Most projects just use the binary
library, not seeing the warnings at all.
But the situation is special for gnulib since it is a source code library.

> 
>>> 2) For the problem of uninitialized variables that lead to undefined
>>>behaviour, I don't see a GCC option that would warn about them [1].
>>>Instead, 'valgrind' is the ultimate tool for detecting these kinds
>>>of problems.
>>>So if someone has a habit of looking only at GCC warnings, they should
>>>change their habit and also use valgrind once in a while.
>>
>> ... the quality of Valgrind
>> depends on the code path coverage - that is not the same as code
>> coverage. To get the test data to cover most code paths you need a
>> fuzzer, at least for the more complex functions / functionality. Writing
>> good fuzzers takes time and sometimes need a lot of human time for
>> tuning.
> 
> I did not say anything negative about fuzzying. Like you say, efforts on
> valgrind testing and efforts on fuzzying are complementary: With the
> fuzzying you increase the code coverage and code path coverage; with
> valgrind you check against undefined behaviour caused by uninitialized
> variables.

You normally fuzz with sanitizers (ASAN, UBSAN) switched on. I remember
only one thing that valgrind found but the sanitizers/fuzzing not.

Uninitialized variables *should* be found by the compiler.

>> since there are
>> tests in glibc/gnulib for glob() that are also used with Valgrind,
>> aren't there ?
> 
> We do have a problem with the valgrind integration into projects that use
> Automake: There's not one clearly best way to use valgrind that is documented,
> therefore every package maintainer spends time on a valgrind integration.

Not sure how much part automake takes here. The way to use valgrind
heavily depends on the test suite. E.g. in wget we only want to test the
wget utility which gets called by the tests itself. So we check a
certain env var and add valgrind to the system()/popen() command line.
How can automake know about that ?

With Best Regards, Tim



signature.asc
Description: OpenPGP digital signature

Re: Warning in spawn-pipe.c (create_pipe)

[Tim Rühsen]

> #if defined __clang__
> # define NOWARN(a) _Pragma( STRINGIFY( clang diagnostic ignored a ) )
> # define NOWARN_PUSH(a) \
>   _Pragma( STRINGIFY( clang diagnostic push ) ) \
>   _Pragma( STRINGIFY( clang diagnostic ignored a ) )
> # define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )

^^ last second typo here - replace 'gcc' with 'clang'



signature.asc
Description: OpenPGP digital signature

Re: Warning in spawn-pipe.c (create_pipe)

[Tim Rühsen]

On 12/15/2017 12:28 AM, Bruno Haible wrote:
Hi Bruno,

>>> Or through an in-file pragma that specifically documents that we are
>>> intentionally ignoring the warning.
>>
>> I vote for this.
> 
> Does this patch eliminate the warning?
> 
> I couldn't reproduce the issue with clang 3.9.1 on Linux, therefore I have
> to ask you to test it.

It works, but... ;-)

Your patch disables that warning for the whole file. IMO, we should keep
the scope of the #pragma as narrow as possible.

We could have a pragma.h include file with the following C99 code (I
leave the clang and gcc version checks to you):

#define STRINGIFY(a) #a

#if defined __clang__
# define NOWARN(a) _Pragma( STRINGIFY( clang diagnostic ignored a ) )
# define NOWARN_PUSH(a) \
  _Pragma( STRINGIFY( clang diagnostic push ) ) \
  _Pragma( STRINGIFY( clang diagnostic ignored a ) )
# define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )
#elif defined __GNUC__
# define NOWARN(a) _Pragma( STRINGIFY( gcc diagnostic ignored a ) )
# define NOWARN_PUSH(a) \
  _Pragma( STRINGIFY( gcc diagnostic push ) ) \
  _Pragma( STRINGIFY( gcc diagnostic ignored a ) )
# define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )
#else
# define NOWARN
# define NOWARN_PUSH
# define NOWARN_POP
#endif

Now we could include that file where needed and make use of the defines like

NOWARN("-Wconditional-uninitialized")

or to narrow the scope

NOWARN_PUSH("-Wconditional-uninitialized")
... code ...
NOWARN_POP


WDYT ?

Regards, Tim



signature.asc
Description: OpenPGP digital signature