Failing misc/check-installed-headers-c with new kernel headers.

Failing misc/check-installed-headers-c with new kernel headers.

[Carlos O'Donell]

In the latest Fedora Rawhide weekly glibc rebase I'm seeing
a failure across all arches.

This:
BUILDSTDERR: FAIL: misc/check-installed-headers-c

With this specific output:
BUILDSTDERR: :: sys/sysctl.h
BUILDSTDERR: ::::
BUILDSTDERR: *** Obsolete types detected:
BUILDSTDERR: /usr/include/linux/sysctl.h:       KERN_PANIC_PRINT=78, /* ulong: bitmask to print system info on panic */
BUILDSTDERR: :::: -D_DEFAULT_SOURCE=1
BUILDSTDERR: :::: -D_GNU_SOURCE=1
BUILDSTDERR: :::: -D_XOPEN_SOURCE=700
BUILDSTDERR: :::: -std=c89
BUILDSTDERR: :::: -std=c89 -D_DEFAULT_SOURCE=1
BUILDSTDERR: :::: -std=c89 -D_GNU_SOURCE=1
BUILDSTDERR: :::: -std=c89 -D_XOPEN_SOURCE=700
BUILDSTDERR: :::: -std=gnu89
BUILDSTDERR: :::: -std=gnu89 -D_DEFAULT_SOURCE=1
BUILDSTDERR: :::: -std=gnu89 -D_GNU_SOURCE=1
BUILDSTDERR: :::: -std=gnu89 -D_XOPEN_SOURCE=700
BUILDSTDERR: :::: -std=c11
BUILDSTDERR: :::: -std=c11 -D_DEFAULT_SOURCE=1
BUILDSTDERR: :::: -std=c11 -D_GNU_SOURCE=1
BUILDSTDERR: :::: -std=c11 -D_XOPEN_SOURCE=700
BUILDSTDERR: :::: -std=gnu11
BUILDSTDERR: :::: -std=gnu11 -D_DEFAULT_SOURCE=1
BUILDSTDERR: :::: -std=gnu11 -D_GNU_SOURCE=1
BUILDSTDERR: :::: -std=gnu11 -D_XOPEN_SOURCE=700

This is a false positive from the regexp used to search for obsolete types.

Commit 81c9d43f94870 is the upstream kernel commit that triggers this:

diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
index d71013fffaf6..87aa2a6d9125 100644
--- a/include/uapi/linux/sysctl.h
+++ b/include/uapi/linux/sysctl.h
@@ -153,6 +153,7 @@ enum
        KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
        KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
        KERN_PANIC_ON_WARN=77, /* int: call panic() in WARN() functions */
+       KERN_PANIC_PRINT=78, /* ulong: bitmask to print system info on panic */
 };
 
 
---

The obsolete type is shorthand in a comment.

Should we expand the regexp in some way to ignore text within single-line
comments that we can easily detect?

Or is the accepted fix something like this (untested)? Seems a shame not to check
sysctl.h. However use of legacy types seem fairly rare these days.

diff --git a/scripts/check-installed-headers.sh b/scripts/check-installed-headers.sh
index 8e7beffd82..1742c2bf20 100644
--- a/scripts/check-installed-headers.sh
+++ b/scripts/check-installed-headers.sh
@@ -127,6 +127,12 @@ EOF
                 ;;
             esac
             ;;
+
+       # linux/sysctl.h is unsupported for all arches because it contains
+       # a comment with 'ulong' as shorthand for a type.
+       (sys/sysctl.h)
+           continue;;
+
     esac
 
     echo :: "$header"
---

Thoughts?

-- 
Cheers,
Carlos.

Re: Failing misc/check-installed-headers-c with new kernel headers.

[Florian Weimer]

* Carlos O'Donell:

> Should we expand the regexp in some way to ignore text within single-line
> comments that we can easily detect?

I think we should use #pragma GCC poison in $cih_test_c, like this:

diff --git a/scripts/check-installed-headers.sh b/scripts/check-installed-headers.sh
index 8e7beffd82..f9e39eb995 100644
--- a/scripts/check-installed-headers.sh
+++ b/scripts/check-installed-headers.sh
@@ -144,6 +144,12 @@ EOF
    inappropriate for this test.  */
 #undef _LIBC
 #undef _GNU_SOURCE
+
+/* Obsolete type names.  */
+#pragma GCC poison ushort
+#pragma GCC poison ulong
+#pragma GCC poison uint
+
 /* The library mode is selected here rather than on the command line to
    ensure that this selection wins. */
 $expanded_lib_mode

And then get rid of the grep.

Thanks,
Florian

Re: Failing misc/check-installed-headers-c with new kernel headers.

[Carlos O'Donell]

On 1/16/19 4:21 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> Should we expand the regexp in some way to ignore text within single-line
>> comments that we can easily detect?
> 
> I think we should use #pragma GCC poison in $cih_test_c, like this:
> 
> diff --git a/scripts/check-installed-headers.sh b/scripts/check-installed-headers.sh
> index 8e7beffd82..f9e39eb995 100644
> --- a/scripts/check-installed-headers.sh
> +++ b/scripts/check-installed-headers.sh
> @@ -144,6 +144,12 @@ EOF
>     inappropriate for this test.  */
>  #undef _LIBC
>  #undef _GNU_SOURCE
> +
> +/* Obsolete type names.  */
> +#pragma GCC poison ushort
> +#pragma GCC poison ulong
> +#pragma GCC poison uint
> +
>  /* The library mode is selected here rather than on the command line to
>     ensure that this selection wins. */
>  $expanded_lib_mode
> 
> And then get rid of the grep.

I like this solution, but it drastically complicates the test.

It's certainly more robust than the regexp.

The problem is that you have to split the tail part of the
test into 2 compilations.

One to test for obsolete types, and another to test for the
correct compilation under the std/macro test matrix.

You can no longer keep the two together because the poison
always triggers on the old types in the base headers.

And when you're looking for poisoning you now have to go
through all the raised compiler errors just like you would
for headers, and exclude any from headers that are allowed
to have the obsolete types.

I guess all-in-all it's more robust, but more work than
I was expecting :/

-- 
Cheers,
Carlos.

Re: Failing misc/check-installed-headers-c with new kernel headers.

[Florian Weimer]

* Carlos O'Donell:

> On 1/16/19 4:21 PM, Florian Weimer wrote:
>> * Carlos O'Donell:
>> 
>>> Should we expand the regexp in some way to ignore text within single-line
>>> comments that we can easily detect?
>> 
>> I think we should use #pragma GCC poison in $cih_test_c, like this:
>> 
>> diff --git a/scripts/check-installed-headers.sh b/scripts/check-installed-headers.sh
>> index 8e7beffd82..f9e39eb995 100644
>> --- a/scripts/check-installed-headers.sh
>> +++ b/scripts/check-installed-headers.sh
>> @@ -144,6 +144,12 @@ EOF
>>     inappropriate for this test.  */
>>  #undef _LIBC
>>  #undef _GNU_SOURCE
>> +
>> +/* Obsolete type names.  */
>> +#pragma GCC poison ushort
>> +#pragma GCC poison ulong
>> +#pragma GCC poison uint
>> +
>>  /* The library mode is selected here rather than on the command line to
>>     ensure that this selection wins. */
>>  $expanded_lib_mode
>> 
>> And then get rid of the grep.
>
> I like this solution, but it drastically complicates the test.

It does not work because use in an unexpanded macro does not count as
use for poisoning.  I find that rather surprising, based on the
documentation of the pragma.

Thanks,
Florian

Re: Failing misc/check-installed-headers-c with new kernel headers.

[DJ Delorie]

I think ignoring comments is the right way to go, else we can't allow a
comment like:

  /* Don't use ulong here, it's obsolete.  */

I don't ever think it's a good idea for commentary to limit technical
choice.

I won't comment on how hard expanding a regex might be ;-)

[RFC] Ignore source code comments in scripts/check-installed-headers.sh

[Tulio Magno Quites Machado Filho]

DJ Delorie <dj@redhat.com> writes:

> I think ignoring comments is the right way to go, else we can't allow a
> comment like:
>
>   /* Don't use ulong here, it's obsolete.  */
>
> I don't ever think it's a good idea for commentary to limit technical
> choice.
>
> I won't comment on how hard expanding a regex might be ;-)

Or we could reuse the preprocessor to do that for us... ;-)

Carlos, could you test if this works in the case you found, please?

---8<---

Use the preprocessor to remove source code comments from header files
before testing if they have any obsolete type.

2019-01-16  Tulio Magno Quites Machado Filho  <tuliom@linux.ibm.com>

	* scripts/check-installed-headers.sh: Pre-process header files
	before checking for obsolete types.

Signed-off-by: Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com>
---
 scripts/check-installed-headers.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/check-installed-headers.sh b/scripts/check-installed-headers.sh
index 8e7beffd82..291774c696 100644
--- a/scripts/check-installed-headers.sh
+++ b/scripts/check-installed-headers.sh
@@ -158,7 +158,9 @@ EOF
                     # Don't repeat work.
                     eval 'case "$h" in ('"$already"') continue;; esac'
 
-                    if grep -qE "$obsolete_type_re" "$h"; then
+                    # Use the preprocessor to remove comments in the source.
+                    if $cc_cmd -E -fpreprocessed "$h" -o - \
+                               | grep -qE "$obsolete_type_re"; then
                         echo "*** Obsolete types detected:"
                         grep -HE "$obsolete_type_re" "$h"
                         failed=1
-- 
2.14.5

Re: [RFC] Ignore source code comments in scripts/check-installed-headers.sh

[DJ Delorie]

Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com> writes:
> Or we could reuse the preprocessor to do that for us... ;-)

No, because we *do* want to check for things like:

#define ulong unsigned long

ulong x;

Re: [RFC] Ignore source code comments in scripts/check-installed-headers.sh

[Tulio Magno Quites Machado Filho]

DJ Delorie <dj@redhat.com> writes:

> Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com> writes:
>> Or we could reuse the preprocessor to do that for us... ;-)
>
> No, because we *do* want to check for things like:
>
> #define ulong unsigned long
>
> ulong x;

With -fpreprocessed, we can still check for that:

:: test.h
::::
*** Obsolete types detected:
./test.h:#define ulong unsigned long
./test.h:ulong x;

-- 
Tulio Magno

Re: [RFC] Ignore source code comments in scripts/check-installed-headers.sh

[DJ Delorie]

Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com> writes:
> With -fpreprocessed, we can still check for that:
>
> :: test.h
> ::::
> *** Obsolete types detected:
> ./test.h:#define ulong unsigned long
> ./test.h:ulong x;

I just tested this (gcc 4 and 7), and the #define didn't survive the
trip...

$ cat foo.c
#define ulong unsigned long

ulong x;  /* don't use ulong */

$ gcc -E -fpreprocessed foo.c
# 3 "foo.c"
ulong x;

Re: [RFC] Ignore source code comments in scripts/check-installed-headers.sh

[Tulio Magno Quites Machado Filho]

DJ Delorie <dj@redhat.com> writes:

> I just tested this (gcc 4 and 7), and the #define didn't survive the
> trip...
>
> $ cat foo.c
> #define ulong unsigned long
>
> ulong x;  /* don't use ulong */
>
> $ gcc -E -fpreprocessed foo.c
> # 3 "foo.c"
> ulong x;

Yep.  You're right.
We're back to the regex.  :-D

-- 
Tulio Magno

Re: [RFC] Ignore source code comments in scripts/check-installed-headers.sh

[Szabolcs Nagy]

On 17/01/2019 00:14, Tulio Magno Quites Machado Filho wrote:
> DJ Delorie <dj@redhat.com> writes:
> 
>> I just tested this (gcc 4 and 7), and the #define didn't survive the
>> trip...
>>
>> $ cat foo.c
>> #define ulong unsigned long
>>
>> ulong x;  /* don't use ulong */
>>
>> $ gcc -E -fpreprocessed foo.c
>> # 3 "foo.c"
>> ulong x;
> 
> Yep.  You're right.
> We're back to the regex.  :-D

-Dd works for me:

$ cat foo.c
#define ulong unsigned long

ulong x; /* don't use ulong */

$ gcc -E -dD foo.c
# 1 "foo.c"
# 1 "<built-in>"
... gcc predefined macros ...
# 1 "<command-line>"
# 31 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 19 "/usr/include/stdc-predef.h" 3 4
#define _STDC_PREDEF_H 1
# 38 "/usr/include/stdc-predef.h" 3 4
#define __STDC_IEC_559__ 1







#define __STDC_IEC_559_COMPLEX__ 1
# 58 "/usr/include/stdc-predef.h" 3 4
#define __STDC_ISO_10646__ 201706L


#define __STDC_NO_THREADS__ 1
# 32 "<command-line>" 2
# 1 "foo.c"
#define ulong unsigned long

unsigned long x;

Re: [RFC] Ignore source code comments in scripts/check-installed-headers.sh

[Szabolcs Nagy]

On 17/01/2019 10:06, Szabolcs Nagy wrote:
> On 17/01/2019 00:14, Tulio Magno Quites Machado Filho wrote:
>> DJ Delorie <dj@redhat.com> writes:
>>
>>> I just tested this (gcc 4 and 7), and the #define didn't survive the
>>> trip...
>>>
>>> $ cat foo.c
>>> #define ulong unsigned long
>>>
>>> ulong x;  /* don't use ulong */
>>>
>>> $ gcc -E -fpreprocessed foo.c
>>> # 3 "foo.c"
>>> ulong x;
>>
>> Yep.  You're right.
>> We're back to the regex.  :-D
> 
> -Dd works for me:

ah you wanted to avoid preprocessing.
never mind then.

Re: Failing misc/check-installed-headers-c with new kernel headers.

[Zack Weinberg]

On Wed, Jan 16, 2019 at 4:11 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> In the latest Fedora Rawhide weekly glibc rebase I'm seeing
> a failure across all arches.
...
> BUILDSTDERR: *** Obsolete types detected:
> BUILDSTDERR: /usr/include/linux/sysctl.h:       KERN_PANIC_PRINT=78, /* ulong: bitmask to print system info on panic */
...
> This is a false positive from the regexp used to search for obsolete types.

I propose to fix this by separating the obsolete-typedefs test to a
new program, implemented in Python, which lexes C accurately enough
that it won't get false positives on the contents of strings and
comments.  It will also not look at headers that aren't under our
control.  Patch shortly.

zw