Re: warnings in unit tests
On Tue, Jun 08, 2021 at 10:56:33AM +0200, Bruno Haible wrote: > Jim Meyering wrote: > > I can live without -Wmissing-prototypes in gnulib tests, but I still > > remember times where using that option exposed a real bug. > > -Wmissing-prototypes typically exposes real bugs when a program is composed > of several compilation units. Unit tests are typically a single compilation > unit plus libtests.a, and libtests.a being built from modules with .h / .c > combinations it does not have the kind of bug that -Wmissing-prototypes can > detect. Unlike many other gcc warnings, -Wmissing-prototypes is especially useful because it doesn't report false positives, so I don't see why one may want to turn -Wmissing-prototypes off. In case of recurse_1(), the function isn't declared static for a specific reason that isn't obvious for casual readers. In such cases it's usually a good idea to add a comment explaining why this case is different from the common pattern. -- ldv
Re: warnings in unit tests
For what it's worth I'm more with Bruno on this. For the tests, the cost of these warnings outweighs the benefit. It'd be OK with me to disable the troublesome warnings globally for the tests subdirectory, using -Wno-missing-prototypes or whatever. For the -Wnull-dereference issue it may be worthwhile to use a circumlocution that fools GCC into not issuing the warning. After all, a compiler smart enough to warn about '*(volatile int *) 0 = 42' might also be smart enough to see that it's undefined behavior and therefore omit the assignment's effect entirely. Perhaps something like the attached (untested) patch? diff --git a/tests/test-sigsegv-catch-stackoverflow2.c b/tests/test-sigsegv-catch-stackoverflow2.c index b94d1310b..bfd4617a3 100644 --- a/tests/test-sigsegv-catch-stackoverflow2.c +++ b/tests/test-sigsegv-catch-stackoverflow2.c @@ -50,6 +50,7 @@ sigset_t mainsigset; volatile int pass = 0; uintptr_t page; +int volatile *null_pointer_to_volatile; static void stackoverflow_handler_continuation (void *arg1, void *arg2, void *arg3) @@ -183,7 +184,7 @@ main () *(volatile int *) (page + 0x678) = 42; break; case 3: - *(volatile int *) 0 = 42; + *null_pointer_to_volatile = 42; break; case 4: break;
Re: warnings in unit tests
Jeffrey Walton wrote: > Apple's port of Clang enables missing prototypes by default. You will > get the warnings whether you use -Wmissing-prototypes or not. You now > have to actively disable the warning with -Wno-missing-prototypes. Thanks for the heads-up. From my perspective, we can treat Apple cc like MSVC, xlc, Sun cc, clang on Windows, and the other vendor compilers. Namely, quickly glance over the build log to see whether there are warnings that look relevant and dangerous, and other than that ignore the warnings. This holds for both lib/ and tests/. Reminder: We never promised a warning-free build, neither of lib/ nor tests/. The difference between lib/ and tests/ is that code in lib/ goes into the binaries delivered by the packages, and therefore if a package maintainer makes an effort to silence a warning, we will consider their patch. Whereas for tests, as I said, it is too much of an effort/cost to do so. Bruno
Re: warnings in unit tests
Jim Meyering wrote: > I can live without -Wmissing-prototypes in gnulib tests, but I still > remember times where using that option exposed a real bug. -Wmissing-prototypes typically exposes real bugs when a program is composed of several compilation units. Unit tests are typically a single compilation unit plus libtests.a, and libtests.a being built from modules with .h / .c combinations it does not have the kind of bug that -Wmissing-prototypes can detect. Anyway, the main point is that I, as the author of 75% of the Gnulib tests and maintainer of the Gnulib tests, trust a certain set of GCC warnings (namely, '-Wall -ftrapv'), as they have proven useful for these tests. If a maintainer of a different package trusts a different set of GCC warnings or clang warnings or the warnings of some other tool, they are welcome to report bugs that they found this way. But they are not welcome to force their preferred set of warning options onto the Gnulib tests, because that means additional maintenance costs, which goes against the goal of having a high test coverage. > My point about the cost/benefit was regarding that 3-line addition for > a single, deliberate NULL-deref. > That one really does not deserve to quash -Wnull-dereference for all tests. Compilers are getting more and more knowledge about POSIX functions. Over time, they will warn about more and more of the corner cases that the Gnulib test suite exercises. So, it's not only about the deliberate pointer access here. Jeffrey Walton is doing sanitizer-enabled testing of Gnulib. This has proven to be more useful than listening to large amounts of GCC or clang warning options. Bruno
Re: [PATCH] mountlist: recognize fuse.portal as dummy file system
On Tuesday, June 8, 2021 12:16:36 AM CEST Pádraig Brady wrote: > On 07/06/2021 13:43, Kamil Dudka wrote: > > This was originally proposed at: > > https://lists.gnu.org/archive/html/bug-gnulib/2021-02/msg00053.html > > > > As the full review might take some time, would it be possible to apply > > at least the part related to fuse.portal file systems? They started to > > > > cause problems recently: > > https://bugs.launchpad.net/ubuntu/+source/xdg-desktop-portal/+bug/190 > > 5623 > > https://github.com/muesli/duf/issues/35 > > https://bugzilla.redhat.com/1913358 > > > > --- > > > > lib/mountlist.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/lib/mountlist.c b/lib/mountlist.c > > index f5d1364c1b..e4c1779829 100644 > > --- a/lib/mountlist.c > > +++ b/lib/mountlist.c > > @@ -169,6 +169,7 @@ > > > > || strcmp (Fs_type, "debugfs") == 0 \ > > || strcmp (Fs_type, "devpts") == 0 \ > > || strcmp (Fs_type, "fusectl") == 0 \ > > > > + || strcmp (Fs_type, "fuse.portal") == 0 \ > > > > || strcmp (Fs_type, "mqueue") == 0 \ > > || strcmp (Fs_type, "rpc_pipefs") == 0 \ > > || strcmp (Fs_type, "sysfs") == 0\ > > Pushed. > "fuse.portal" should be fine to always consider as dummy. Thanks! Kamil
