[PATCH] non-gcc-specific exception.sh

[PATCH] non-gcc-specific exception.sh

[Michael Shigorin]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]

	Hello,
please find attached the patch to (hopefully) improve
self-surgery script that uses gcc-specific -aux-info now.
Should help clang, icc and the like (in my case there's
no proper gcc port for the target platform just yet).

-- 
 ---- WBR, Michael Shigorin / http://altlinux.org
  ------ http://opennet.ru / http://anna-news.info

[-- Attachment #2: 0001-exception.sh-fixed-to-work-with-non-gcc.patch --]
[-- Type: text/x-patch, Size: 2005 bytes --]

From ccfa41adf46367e132ba7556fad53dfce9ddffaf Mon Sep 17 00:00:00 2001
From: Michael Shigorin <mike@altlinux.org>
Date: Mon, 7 Oct 2019 16:00:39 +0300
Subject: [PATCH] exception.sh: fixed to work with non-gcc

-aux-info is a gccism absent in clang, icc and the like;
let's reimplement this in a slightly cleaner way with -E.

Suggested-by: Evgeny Mareev
---
 libselinux/src/exception.sh  | 12 +++++-------
 libsemanage/src/exception.sh | 12 +++++-------
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/libselinux/src/exception.sh b/libselinux/src/exception.sh
index d6c8c71..459295c 100755
--- a/libselinux/src/exception.sh
+++ b/libselinux/src/exception.sh
@@ -15,10 +15,8 @@ echo "
 ;;
 esac
 }
-if ! ${CC:-gcc} -x c -c -I../include - -aux-info temp.aux < ../include/selinux/selinux.h
-then
-    # clang does not support -aux-info so fall back to gcc
-    gcc -x c -c -I../include - -aux-info temp.aux < ../include/selinux/selinux.h
-fi
-for i in `awk '/<stdin>.*extern int/ { print $6 }' temp.aux`; do except $i ; done 
-rm -f -- temp.aux -.o
+
+# equivalent of gcc-specific -aux-info
+${CC:-gcc} -E -I../include -xc ../include/selinux/selinux.h |
+	sed -n 's/^extern * int *\(\w*\) *(.*$/\1/p' |
+	while read f; do except $f; done
diff --git a/libsemanage/src/exception.sh b/libsemanage/src/exception.sh
index 97bc2ae..11586a1 100644
--- a/libsemanage/src/exception.sh
+++ b/libsemanage/src/exception.sh
@@ -9,10 +9,8 @@ echo "
 }
 "
 }
-if ! ${CC:-gcc} -x c -c -I../include - -aux-info temp.aux < ../include/semanage/semanage.h
-then
-    # clang does not support -aux-info so fall back to gcc
-    gcc -x c -c -I../include - -aux-info temp.aux < ../include/semanage/semanage.h
-fi
-for i in `awk '/extern int/ { print $6 }' temp.aux`; do except $i ; done
-rm -f -- temp.aux -.o
+
+# equivalent of gcc-specific -aux-info
+${CC:-gcc} -E -I../include -xc ../include/semanage/semanage.h |
+	sed -n 's/^extern * int *\(\w*\) *(.*$/\1/p' |
+	while read f; do except $f; done
-- 
2.10.4

Re: [PATCH] non-gcc-specific exception.sh

[Nicolas Iooss]

On Mon, Oct 7, 2019 at 3:20 PM Michael Shigorin <mike@altlinux.org> wrote:
>
>         Hello,
> please find attached the patch to (hopefully) improve
> self-surgery script that uses gcc-specific -aux-info now.
> Should help clang, icc and the like (in my case there's
> no proper gcc port for the target platform just yet).
>
> --
>  ---- WBR, Michael Shigorin / http://altlinux.org
>   ------ http://opennet.ru / http://anna-news.info

Hello,
How did you test this patch? On my system (Arch Linux x86-64), I get
the following differences in the generated list of functions, for
libselinux:

+select
+pselect
-selinuxfs_exists

This is because /usr/include/sys/select.h contains "extern int select
(int __nfds, fd_set *__restrict __readfds," and "extern int pselect
(int __nfds, fd_set *__restrict __readfds,", and because
libselinux/include/selinux/selinux.h contains "int
selinuxfs_exists(void);" without "extern". Your patch therefore
changes things in a way that seems unintended.

I nevertheless agree that making the build system use clang when
CC=clang is a good idea. As the regular expression you sent is quite
fragile, a possible way of preventing issues such as the differences I
found is to try using both methods (-aux-info and -E) in a "make test"
target and fail with a fatal error when they do not produce the same
output (this fatal error would be caught by a continuous-integration
system, which would make the developers aware of something wrong).

Moreover, please send your patch inline, if possible (for example with
"git send-email"), and add a "Signed-off-by:" line as documented in
https://github.com/SELinuxProject/selinux/blob/master/CONTRIBUTING.md#contributing-code.

Thanks,
Nicolas

Re: [PATCH] non-gcc-specific exception.sh

[Michael Shigorin]

On Mon, Oct 07, 2019 at 06:27:29PM +0200, Nicolas Iooss wrote:
> > please find attached the patch to (hopefully) improve
> > self-surgery script that uses gcc-specific -aux-info now.
> > Should help clang, icc and the like (in my case there's
> > no proper gcc port for the target platform just yet).
> How did you test this patch? On my system (Arch Linux x86-64),
> I get the following differences in the generated list of
> functions, for libselinux:
> 
> +select
> +pselect
> -selinuxfs_exists

selinuxfs_exists is declared as just int, not external int, and
gets filtered out with awk one-liner within the original script.

I've spotted select/pselect and have paid some attention to
zeroing the diff *but* overlooked the <stdin> filter in the
script from libselinux-2.9 tagged tree I started looking at.
Evgeny suggested doing something about the changed source
marker format either, shame on me for distracting.

> This is because /usr/include/sys/select.h contains "extern int select
> (int __nfds, fd_set *__restrict __readfds," and "extern int pselect
> (int __nfds, fd_set *__restrict __readfds,", and because
> libselinux/include/selinux/selinux.h contains "int
> selinuxfs_exists(void);" without "extern". Your patch therefore
> changes things in a way that seems unintended.

Will this one do better?  Looks a bit messy though[1]:

gcc -E -I../include -xc ../include/selinux/selinux.h |
  awk -F '[ (]' '/^#/{p=0} \
    /^# .*selinux\/selinux\.h/{p=1} \
    /^extern int/{if(p==1){print $3}}'

> As the regular expression you sent is quite fragile a possible
> way of preventing issues such as the differences I found is
> to try using both methods (-aux-info and -E) in a "make test"
> target and fail with a fatal error when they do not produce
> the same output (this fatal error would be caught by a
> continuous-integration system, which would make the developers
> aware of something wrong).

Good test for those who have it handy, my primary intent was
to be able to at least build without (not-yet-ported) gcc.

> Moreover, please send your patch inline, if possible (for example with
> "git send-email"), and add a "Signed-off-by:" line as documented in
> https://github.com/SELinuxProject/selinux/blob/master/CONTRIBUTING.md#contributing-code.

Thank you for the review and suggestions, please have a look
at the proposed handler replacement; if it's ok I'll arrange
it as a commit and hopefully figure out the test tomorrow,
getting sleepy by now...

---

[1] start/end patterns don't cut it for me given repetitions:

sed -n '/^# .*selinux\/selinux\.h/,/^#/{s/^extern int \(\w*\) *(.*$/\1/p}'

skips e.g. security_deny_unknown given gcc's output
but works fine with somewhat differing lcc's one.

-- 
 ---- WBR, Michael Shigorin / http://altlinux.org
  ------ http://opennet.ru / http://anna-news.info

Re: [PATCH] non-gcc-specific exception.sh

[Nicolas Iooss]

On Mon, Oct 7, 2019 at 9:21 PM Michael Shigorin <mike@altlinux.org> wrote:
>
> On Mon, Oct 07, 2019 at 06:27:29PM +0200, Nicolas Iooss wrote:
> > > please find attached the patch to (hopefully) improve
> > > self-surgery script that uses gcc-specific -aux-info now.
> > > Should help clang, icc and the like (in my case there's
> > > no proper gcc port for the target platform just yet).
> > How did you test this patch? On my system (Arch Linux x86-64),
> > I get the following differences in the generated list of
> > functions, for libselinux:
> >
> > +select
> > +pselect
> > -selinuxfs_exists
>
> selinuxfs_exists is declared as just int, not external int, and
> gets filtered out with awk one-liner within the original script.
>
> I've spotted select/pselect and have paid some attention to
> zeroing the diff *but* overlooked the <stdin> filter in the
> script from libselinux-2.9 tagged tree I started looking at.
> Evgeny suggested doing something about the changed source
> marker format either, shame on me for distracting.
>
> > This is because /usr/include/sys/select.h contains "extern int select
> > (int __nfds, fd_set *__restrict __readfds," and "extern int pselect
> > (int __nfds, fd_set *__restrict __readfds,", and because
> > libselinux/include/selinux/selinux.h contains "int
> > selinuxfs_exists(void);" without "extern". Your patch therefore
> > changes things in a way that seems unintended.
>
> Will this one do better?  Looks a bit messy though[1]:
>
> gcc -E -I../include -xc ../include/selinux/selinux.h |
>   awk -F '[ (]' '/^#/{p=0} \
>     /^# .*selinux\/selinux\.h/{p=1} \
>     /^extern int/{if(p==1){print $3}}'
>
> > As the regular expression you sent is quite fragile a possible
> > way of preventing issues such as the differences I found is
> > to try using both methods (-aux-info and -E) in a "make test"
> > target and fail with a fatal error when they do not produce
> > the same output (this fatal error would be caught by a
> > continuous-integration system, which would make the developers
> > aware of something wrong).
>
> Good test for those who have it handy, my primary intent was
> to be able to at least build without (not-yet-ported) gcc.
>
> > Moreover, please send your patch inline, if possible (for example with
> > "git send-email"), and add a "Signed-off-by:" line as documented in
> > https://github.com/SELinuxProject/selinux/blob/master/CONTRIBUTING.md#contributing-code.
>
> Thank you for the review and suggestions, please have a look
> at the proposed handler replacement; if it's ok I'll arrange
> it as a commit and hopefully figure out the test tomorrow,
> getting sleepy by now...

Hello,
I wanted to spend some time testing different approaches before
replying. Your awk command works fine, but I am wondering whether its
complexity is needed at all. For example, why not directly grab the
"extern int" lines in selinux.h (using sed -n s/.../.../p). This would
work well with libselinux and avoid using awk for this simple task,
but not for libsemanage... or it could, with:

    cat ../include/semanage/*.h | \
    sed -n 's/^extern \+int \+\([0-9A-Za-z_]\+\) *(.*$/\1/p'

Actually, the main reason why it is currently not possible to migrate
from "gcc -aux-info and awk" to "cat | sed" is caused by the fact that
the file generated by "gcc -aux-info" adds "extern" before all
function prototypes. Adding "extern" directly to the prototypes of all
exported functions in header files seems to be required in order to
use something else than "gcc -aux-info" to generate the glue code that
handles exception in Python bindings.

Therefore I suggest the following approach:
1. add "extern" to all exported functions in libselinux and
libsemanage public headers (I can send a patch for this tomorrow if
you don't want to do this)
2. change exception.sh to use something else than "gcc -aux-info" to
generate the glue code
3. add something so that "make test" compares the result of "$CC
-aux-info" (if $CC is gcc-compatible) with the new way to find
functions, in order to report differences in the generated glue code.
This could for example consist in adding some code in exception.sh
such that "./exception.sh test" performs this check.

Would this suit you?

By the way, I found out that libsemanage generates glue code for
select() and pselect() too, as it does not filter the output of "gcc
-aux-info". This is a bug that needs to be fixed.

Thanks,
Nicolas

Re: [PATCH] non-gcc-specific exception.sh

[Michael Shigorin]

On Sat, Oct 12, 2019 at 06:55:39PM +0200, Nicolas Iooss wrote:
> > Will this one do better?  Looks a bit messy though[1]:
> >
> > gcc -E -I../include -xc ../include/selinux/selinux.h |
> >   awk -F '[ (]' '/^#/{p=0} \
> >     /^# .*selinux\/selinux\.h/{p=1} \
> >     /^extern int/{if(p==1){print $3}}'
> I wanted to spend some time testing different approaches before
> replying. Your awk command works fine, but I am wondering
> whether its complexity is needed at all.

I've considered this approach too but decided to try and mimic
what's done already first, and maybe try the next step afterwards
(albeit "a temporary solution is the most permanent one" is a well
known effect indeed :-).

> For example, why not directly grab the "extern int" lines in
> selinux.h (using sed -n s/.../.../p). This would work well with
> libselinux and avoid using awk for this simple task, but not
> for libsemanage...  or it could, with:
> 
>     cat ../include/semanage/*.h | \
>     sed -n 's/^extern \+int \+\([0-9A-Za-z_]\+\) *(.*$/\1/p'
> 
> Actually, the main reason why it is currently not possible to migrate
> from "gcc -aux-info and awk" to "cat | sed" is caused by the fact that
> the file generated by "gcc -aux-info" adds "extern" before all
> function prototypes. Adding "extern" directly to the prototypes of all
> exported functions in header files seems to be required in order to
> use something else than "gcc -aux-info" to generate the glue code that
> handles exception in Python bindings.

That's another part I'm not strong at, being rather shell/make
sysadmin/maintainer type than a proper C developer; only spotted
the selinuxfs_exists prototype difference, in particular.

> Therefore I suggest the following approach:
> 1. add "extern" to all exported functions in libselinux and
> libsemanage public headers (I can send a patch for this
> tomorrow if you don't want to do this)

I'd be grateful to you as touching code I have no clue about
is something I try to avoid.

> 2. change exception.sh to use something else than "gcc -aux-info"
> to generate the glue code

This is what I can take care of, -E way or through direct parsing.

> 3. add something so that "make test" compares the result of "$CC
> -aux-info" (if $CC is gcc-compatible) with the new way to find
> functions, in order to report differences in the generated glue code.
> This could for example consist in adding some code in exception.sh
> such that "./exception.sh test" performs this check.

I can do that either given your support with review.

> Would this suit you?

Definitely.

> By the way, I found out that libsemanage generates glue code
> for select() and pselect() too, as it does not filter the
> output of "gcc -aux-info". This is a bug that needs to be
> fixed.

~/git/libselinux/libsemanage/src> touch FILTER-OUT-p_select-TOO    

-- 
 ---- WBR, Michael Shigorin / http://altlinux.org
  ------ http://opennet.ru / http://anna-news.info