Re: [PATCH] Kill regexp.h

["Carlos O'Donell" <carlos at redhat dot com>]

On 07/12/2015 03:27 PM, Zack Weinberg wrote:
> <regexp.h> (not to be confused with <regex.h>) is an obsolete and
> frankly horrible regular expression-matching API.  It was part of SVID
> but was withdrawn in Issue 5 (for reference, we're on Issue 7 now).
> It doesn't do anything you can't do with <regex.h>, and using it
> involves defining a bunch of macros before including the header.
> Moreover, the code in regexp.h that uses those macros has been buggy
> since its creation (in 1996) and no one has noticed, which indicates
> to me that there are no users.  (Specifically, RETURN() is used in a
> whole bunch of cases where it should have been ERROR().)
> 
> Therefore, this patch stubs out the header and demotes the
> implementation to compatibility symbols.  I hope this is not too late
> to squeeze into glibc 2.22.  (Note: there isn't any predefined macro
> to put .bss symbols in a compatibility subsection.  Using
> 'attribute_data_compat_section' would make the libc on disk bigger,
> which seems like a move in the wrong direction.  It's only three
> pointers' worth of junk, anyway...)

Too late for 2.22. You posted on the 12th which was technically frozen.
Please keep discussing for 2.23.

> The ABI checker does not appear to cover these symbols; I manually
> tested the effect of the patch as follows:

Interesting. I would have expected the ABI symbol checker to catch these
symbols... but I haven't looked yet.

Do you have any explanation for that? A manual test is not exactly what
we want to see going forward.

> # glibc 2.19 (Debian)
> $ readelf --dyn-syms /lib/x86_64-linux-gnu/libc.so.6 | grep -E 'advance|step|loc[12s]'
>    540: 00000000000e4da0   104 FUNC    WEAK   DEFAULT   12 step@@GLIBC_2.2.5
>   1364: 00000000000e4e10    84 FUNC    WEAK   DEFAULT   12 advance@@GLIBC_2.2.5
>   2051: 00000000003a85e8     8 OBJECT  GLOBAL DEFAULT   32 loc1@@GLIBC_2.2.5
>   2054: 00000000003a85f0     8 OBJECT  GLOBAL DEFAULT   32 loc2@@GLIBC_2.2.5
>   2198: 00000000003a85f8     8 OBJECT  GLOBAL DEFAULT   32 locs@@GLIBC_2.2.5
> 
> # patched libc
> $ readelf --dyn-syms ./libc.so.6 | grep -E 'advance|step|loc[12s]'
>    541: 000000000011eb80   104 FUNC    WEAK   DEFAULT   12 step@GLIBC_2.2.5
>   1374: 000000000011ebf0    84 FUNC    WEAK   DEFAULT   12 advance@GLIBC_2.2.5
>   2065: 00000000003a35e0     8 OBJECT  GLOBAL DEFAULT   32 loc1@GLIBC_2.2.5
>   2068: 00000000003a35e8     8 OBJECT  GLOBAL DEFAULT   32 loc2@GLIBC_2.2.5
>   2211: 00000000003a35d8     8 OBJECT  GLOBAL DEFAULT   32 locs@GLIBC_2.2.5
> 
> $ cat test.c
> #include <inttypes.h>
> 
> extern char *loc1;
> extern char *loc2;
> extern char *locs;
> extern int step();
> extern int advance();
> 
> int main(void)
> {
>   return (int)((intptr_t)step + (intptr_t)advance + (intptr_t)&locs + (intptr_t)&loc1 + (intptr_t)&loc2);
> }
> 
> $ gcc-5 test.c; echo $?
> 0
> $ ./testrun.sh ./a.out; echo $?
> 136
> $ gcc-5 -nostdlib -nostartfiles csu/crt1.o csu/crti.o `gcc-5 --print-file-name=crtbegin.o` test.c libc.so.6 libc_nonshared.a -lgcc `gcc-5 --print-file-name=crtend.o` csu/crtn.o
> /tmp/ccC6lyqC.o: In function `main':
> test.c:(.text+0x5): undefined reference to `locs'
> test.c:(.text+0xc): undefined reference to `step'
> test.c:(.text+0x13): undefined reference to `loc2'
> test.c:(.text+0x1c): undefined reference to `advance'
> test.c:(.text+0x23): undefined reference to `loc1'
> collect2: error: ld returned 1 exit status
> 
> ---
> 
> N.B. I believe there *is* a past-and-future-changes copyright
> assignment on file for me for glibc, but it was filed long, long
> ago, if I need to do new paperwork that's OK.

The problem is that your past-and-futures-changes were with
zack@rabi.phys.columbia.edu, and we don't know what your present
employer might say about the copyright of your work. Therefore
we do need new paperwork either personal ones for you @panix.com,
or corporate coverage.

At a high level your patch looks OK, it makes sense to deprecate
these interfaces, but I think we should to do this in two stages.
Add warnings and then remove.

Not to mention we need a test to make sure the symbols are at the
versions they should be.

Cheers,
Carlos.