Emil Velikov
2015-Feb-26 14:44 UTC
[LLVMdev] [Mesa-dev] [PATCH 1/2 v3] configure: add visibility macro detection to configure
Hi Marc On 17/02/15 09:40, Marc Dietrich wrote:> This adds clang/gcc visibility macro detection to configure and util/macros.h. > This is can be used to conveniently add e.g. a "HIDDEN" attribute to a function. >I believe this should be OK to go in regardless of the status of patch 2. There are just a couple of trivial nitpicks.> Signed-off-by: Marc Dietrich <marvin24 at gmx.de> > --- > v2: use VISIBILITY_*FLAGS instead of *FLAGS directly > v3: no change > > configure.ac | 28 ++++++---------------------- > src/util/macros.h | 6 ++++++ > 2 files changed, 12 insertions(+), 22 deletions(-) > > diff --git a/configure.ac b/configure.ac > index 351027b..266764a 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -189,6 +189,7 @@ AX_GCC_FUNC_ATTRIBUTE([flatten]) > AX_GCC_FUNC_ATTRIBUTE([format]) > AX_GCC_FUNC_ATTRIBUTE([malloc]) > AX_GCC_FUNC_ATTRIBUTE([packed]) > +AX_GCC_FUNC_ATTRIBUTE([visibility]) > > AM_CONDITIONAL([GEN_ASM_OFFSETS], test "x$GEN_ASM_OFFSETS" = xyes) > > @@ -245,17 +246,13 @@ if test "x$GCC" = xyes; then > AC_MSG_RESULT([yes]), > [CFLAGS="$save_CFLAGS -Wmissing-prototypes"; > AC_MSG_RESULT([no])]); > + CFLAGS=$save_CFLAGS >I'm not sure we want/need this one ?> # Enable -fvisibility=hidden if using a gcc that supports itAs spotted by Sedat, please update the comment.> - save_CFLAGS="$CFLAGS" > - AC_MSG_CHECKING([whether $CC supports -fvisibility=hidden]) > - VISIBILITY_CFLAGS="-fvisibility=hidden" > - CFLAGS="$CFLAGS $VISIBILITY_CFLAGS" > - AC_LINK_IFELSE([AC_LANG_PROGRAM()], AC_MSG_RESULT([yes]), > - [VISIBILITY_CFLAGS=""; AC_MSG_RESULT([no])]); > - > - # Restore CFLAGS; VISIBILITY_CFLAGS are added to it where needed. > - CFLAGS=$save_CFLAGS > + if test "x${ax_cv_have_func_attribute_visibility}" = xyes; then > + VISIBILITY_CFLAGS="-fvisibility=hidden" > + VISIBILITY_CXXFLAGS="-fvisibility=hidden" > + fi >As these two are no longer GCC "specific" we can move them out of the if test x$GCC = xyes, conditional.> # Work around aliasing bugs - developers should comment this out > CFLAGS="$CFLAGS -fno-strict-aliasing" > @@ -267,19 +264,6 @@ fi > if test "x$GXX" = xyes; then > CXXFLAGS="$CXXFLAGS -Wall" > > - # Enable -fvisibility=hidden if using a gcc that supports it > - save_CXXFLAGS="$CXXFLAGS" > - AC_MSG_CHECKING([whether $CXX supports -fvisibility=hidden]) > - VISIBILITY_CXXFLAGS="-fvisibility=hidden" > - CXXFLAGS="$CXXFLAGS $VISIBILITY_CXXFLAGS" > - AC_LANG_PUSH([C++]) > - AC_LINK_IFELSE([AC_LANG_PROGRAM()], AC_MSG_RESULT([yes]), > - [VISIBILITY_CXXFLAGS="" ; AC_MSG_RESULT([no])]); > - AC_LANG_POP([C++]) > - > - # Restore CXXFLAGS; VISIBILITY_CXXFLAGS are added to it where needed. > - CXXFLAGS=$save_CXXFLAGS > - > # Work around aliasing bugs - developers should comment this out > CXXFLAGS="$CXXFLAGS -fno-strict-aliasing" > > diff --git a/src/util/macros.h b/src/util/macros.h > index eec8b93..7682511 100644 > --- a/src/util/macros.h > +++ b/src/util/macros.h > @@ -117,6 +117,12 @@ do { \ > #define PRINTFLIKE(f, a) > #endif > > +#ifdef HAVE_FUNC_ATTRIBUTE_VISIBILITY > +#define HIDDEN __attribute__((visibility("hidden"))) > +#else > +#define HIDDEN > +#endif > +We already define HIDDEN in the asm code. Can you check (build or otherwise) that things don't clash for --enable-asm and --{en,dis}able-shared-glapi. Check the configure log, as we conveniently toggle the latter. Thanks Emil
Marc Dietrich
2015-Feb-26 15:26 UTC
[LLVMdev] [Mesa-dev] [PATCH 1/2 v3] configure: add visibility macro detectionto configure
Am Donnerstag, 26. Februar 2015, 14:44:25 schrieb Emil Velikov:> Hi Marc > > On 17/02/15 09:40, Marc Dietrich wrote: > > This adds clang/gcc visibility macro detection to configure and > > util/macros.h. This is can be used to conveniently add e.g. a "HIDDEN" > > attribute to a function. > I believe this should be OK to go in regardless of the status of patch > 2. There are just a couple of trivial nitpicks.as pointed out, not if there is a better solution. It would be nice if people could test the alternative first.> > Signed-off-by: Marc Dietrich <marvin24 at gmx.de> > > --- > > v2: use VISIBILITY_*FLAGS instead of *FLAGS directly > > v3: no change > > > > configure.ac | 28 ++++++---------------------- > > src/util/macros.h | 6 ++++++ > > 2 files changed, 12 insertions(+), 22 deletions(-) > > > > diff --git a/configure.ac b/configure.ac > > index 351027b..266764a 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -189,6 +189,7 @@ AX_GCC_FUNC_ATTRIBUTE([flatten]) > > > > AX_GCC_FUNC_ATTRIBUTE([format]) > > AX_GCC_FUNC_ATTRIBUTE([malloc]) > > AX_GCC_FUNC_ATTRIBUTE([packed]) > > > > +AX_GCC_FUNC_ATTRIBUTE([visibility]) > > > > AM_CONDITIONAL([GEN_ASM_OFFSETS], test "x$GEN_ASM_OFFSETS" = xyes) > > > > @@ -245,17 +246,13 @@ if test "x$GCC" = xyes; then > > > > AC_MSG_RESULT([yes]), > > [CFLAGS="$save_CFLAGS -Wmissing-prototypes"; > > > > AC_MSG_RESULT([no])]); > > > > + CFLAGS=$save_CFLAGS > > I'm not sure we want/need this one ?it restores the CFLAGS from the test above. In fact I just moved it from the blow upwards. Maybe the diff could be shorter.> > > # Enable -fvisibility=hidden if using a gcc that supports it > > As spotted by Sedat, please update the comment. > > > - save_CFLAGS="$CFLAGS" > > - AC_MSG_CHECKING([whether $CC supports -fvisibility=hidden]) > > - VISIBILITY_CFLAGS="-fvisibility=hidden" > > - CFLAGS="$CFLAGS $VISIBILITY_CFLAGS" > > - AC_LINK_IFELSE([AC_LANG_PROGRAM()], AC_MSG_RESULT([yes]), > > - [VISIBILITY_CFLAGS=""; AC_MSG_RESULT([no])]); > > - > > - # Restore CFLAGS; VISIBILITY_CFLAGS are added to it where needed. > > - CFLAGS=$save_CFLAGS > > + if test "x${ax_cv_have_func_attribute_visibility}" = xyes; then > > + VISIBILITY_CFLAGS="-fvisibility=hidden" > > + VISIBILITY_CXXFLAGS="-fvisibility=hidden" > > + fi > > As these two are no longer GCC "specific" we can move them out of the if > test x$GCC = xyes, conditional.right.> > > # Work around aliasing bugs - developers should comment this out > > CFLAGS="$CFLAGS -fno-strict-aliasing" > > > > @@ -267,19 +264,6 @@ fi > > > > if test "x$GXX" = xyes; then > > > > CXXFLAGS="$CXXFLAGS -Wall" > > > > - # Enable -fvisibility=hidden if using a gcc that supports it > > - save_CXXFLAGS="$CXXFLAGS" > > - AC_MSG_CHECKING([whether $CXX supports -fvisibility=hidden]) > > - VISIBILITY_CXXFLAGS="-fvisibility=hidden" > > - CXXFLAGS="$CXXFLAGS $VISIBILITY_CXXFLAGS" > > - AC_LANG_PUSH([C++]) > > - AC_LINK_IFELSE([AC_LANG_PROGRAM()], AC_MSG_RESULT([yes]), > > - [VISIBILITY_CXXFLAGS="" ; AC_MSG_RESULT([no])]); > > - AC_LANG_POP([C++]) > > - > > - # Restore CXXFLAGS; VISIBILITY_CXXFLAGS are added to it where needed. > > - CXXFLAGS=$save_CXXFLAGS > > - > > > > # Work around aliasing bugs - developers should comment this out > > CXXFLAGS="$CXXFLAGS -fno-strict-aliasing" > > > > diff --git a/src/util/macros.h b/src/util/macros.h > > index eec8b93..7682511 100644 > > --- a/src/util/macros.h > > +++ b/src/util/macros.h > > @@ -117,6 +117,12 @@ do { \ > > > > #define PRINTFLIKE(f, a) > > #endif > > > > +#ifdef HAVE_FUNC_ATTRIBUTE_VISIBILITY > > +#define HIDDEN __attribute__((visibility("hidden"))) > > +#else > > +#define HIDDEN > > +#endif > > + > > We already define HIDDEN in the asm code. > > Can you check (build or otherwise) that things don't clash for > --enable-asm and --{en,dis}able-shared-glapi. Check the configure log, > as we conveniently toggle the latter.I always build with --enable-asm (or better without --disable-asm) and saw no symbol clash. I guess because the required header is not included at the same time. Is this possible at all (asm vs. c files)? Another interesting point is in which case we can build without shared-glapi. I failed to build ES1 or ES2 alone, because it seem to always require glapi. This would mean that the dispatch table always begins with the glapi. Maybe I confuse things, but in this case, just using extern void shared_dispatch_stub_0(); would solve all problems without the HIDDEN hacks. Marc -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: This is a digitally signed message part. URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150226/870b0253/attachment.sig>
Emil Velikov
2015-Feb-26 15:56 UTC
[LLVMdev] [Mesa-dev] [PATCH 1/2 v3] configure: add visibility macro detectionto configure
On 26/02/15 15:26, Marc Dietrich wrote:> Am Donnerstag, 26. Februar 2015, 14:44:25 schrieb Emil Velikov: >> Hi Marc >> >> On 17/02/15 09:40, Marc Dietrich wrote: >>> This adds clang/gcc visibility macro detection to configure and >>> util/macros.h. This is can be used to conveniently add e.g. a "HIDDEN" >>> attribute to a function. >> I believe this should be OK to go in regardless of the status of patch >> 2. There are just a couple of trivial nitpicks. > > as pointed out, not if there is a better solution. It would be nice if people > could test the alternative first. >Can you point out which alternative you have in mind. I'm a bit lost in this thread. ...>>> @@ -245,17 +246,13 @@ if test "x$GCC" = xyes; then >>> >>> AC_MSG_RESULT([yes]), >>> [CFLAGS="$save_CFLAGS -Wmissing-prototypes"; >>> >>> AC_MSG_RESULT([no])]); >>> >>> + CFLAGS=$save_CFLAGS >> >> I'm not sure we want/need this one ? > > it restores the CFLAGS from the test above. In fact I just moved it from the > blow upwards. Maybe the diff could be shorter. >Indeed it seems like it's slightly busted currently. Although the extra line does not seem to make it better though :-( Presently it adds to the final CFLAGS, -Werror=implicit-function-declaration -Werror=missing-prototypes or optionally -Wmissing-prototypes but with your change it won't add either one. ...> I always build with --enable-asm (or better without --disable-asm) and saw no > symbol clash. I guess because the required header is not included at the same > time. Is this possible at all (asm vs. c files)? >Pretty sure you can - #include and #define are preprocessor commands. But as you noticed there was no clash, so we're ok.> Another interesting point is in which case we can build without shared-glapi. > I failed to build ES1 or ES2 alone, because it seem to always require glapi.Yes, to prevent even greater chaos or build permutations we've been mandating shared-glapi if more than one GL* api is selected. To solve this, just drop es{1,2} from the configure line.> This would mean that the dispatch table always begins with the glapi. Maybe I > confuse things, but in this case, just using > extern void shared_dispatch_stub_0(); > would solve all problems without the HIDDEN hacks. >The so called hack is patch 2, afaict. Having a single generic definition of the attribute sounds like good patch regardless of that patch. I'm guessing scons and Android could use a -DHAVE_FUNC_ATTRIBUTE_VISIBILITY somewhere but that can be done as a follow-up. Emil