* [PATCH] non-gcc-specific exception.sh @ 2019-10-07 13:20 Michael Shigorin 2019-10-07 16:27 ` Nicolas Iooss 0 siblings, 1 reply; 5+ messages in thread From: Michael Shigorin @ 2019-10-07 13:20 UTC (permalink / raw) To: selinux; +Cc: Daniel J Walsh, Nicolas Iooss [-- 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] non-gcc-specific exception.sh 2019-10-07 13:20 [PATCH] non-gcc-specific exception.sh Michael Shigorin @ 2019-10-07 16:27 ` Nicolas Iooss 2019-10-07 19:21 ` Michael Shigorin 0 siblings, 1 reply; 5+ messages in thread From: Nicolas Iooss @ 2019-10-07 16:27 UTC (permalink / raw) To: Michael Shigorin; +Cc: SElinux list, Daniel J Walsh 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] non-gcc-specific exception.sh 2019-10-07 16:27 ` Nicolas Iooss @ 2019-10-07 19:21 ` Michael Shigorin 2019-10-12 16:55 ` Nicolas Iooss 0 siblings, 1 reply; 5+ messages in thread From: Michael Shigorin @ 2019-10-07 19:21 UTC (permalink / raw) To: Nicolas Iooss; +Cc: SElinux list, Daniel J Walsh 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] non-gcc-specific exception.sh 2019-10-07 19:21 ` Michael Shigorin @ 2019-10-12 16:55 ` Nicolas Iooss 2019-10-12 17:23 ` Michael Shigorin 0 siblings, 1 reply; 5+ messages in thread From: Nicolas Iooss @ 2019-10-12 16:55 UTC (permalink / raw) To: Michael Shigorin; +Cc: SElinux list 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] non-gcc-specific exception.sh 2019-10-12 16:55 ` Nicolas Iooss @ 2019-10-12 17:23 ` Michael Shigorin 0 siblings, 0 replies; 5+ messages in thread From: Michael Shigorin @ 2019-10-12 17:23 UTC (permalink / raw) To: Nicolas Iooss; +Cc: SElinux list 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-10-12 17:24 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-07 13:20 [PATCH] non-gcc-specific exception.sh Michael Shigorin 2019-10-07 16:27 ` Nicolas Iooss 2019-10-07 19:21 ` Michael Shigorin 2019-10-12 16:55 ` Nicolas Iooss 2019-10-12 17:23 ` Michael Shigorin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).