Skip to contentSkip to conversation

Add a check for -Wl, --wrap support in linker

PATCH In review [email protected]... 4 participants 11 messages Jan 18, 2017 – Jan 20, 2017

lore
Share & tools
lore

[email protected]...

author
#0 lore raw

PATCH 1/1 Add a check for -Wl, --wrap support in linker

From: Selva Nair <[email protected]...>

- Also make tests that require --wrap option to be conditional on this support

Signed-off-by Selva Nair <[email protected]...>
---
 configure.ac                         | 26 ++++++++++++++++++++++++++
 tests/unit_tests/openvpn/Makefile.am |  6 +++++-
 2 files changed, 31 insertions(+), 1 deletion(-)
configure.ac +26 -0
index 43487b0..0b7fea9 100644
@@ -582,6 +582,31 @@ AC_COMPILE_IFELSE(
 	[AC_MSG_RESULT([no])]
 )
 
+saved_LDFLAGS="$LDFLAGS"
+LDFLAGS="$LDFLAGS -Wl,--wrap=exit"
+AC_MSG_CHECKING([linker supports --wrap])
+AC_LINK_IFELSE(
+	[AC_LANG_PROGRAM(
+		[[
+			void exit(int);
+			void __real_exit(int);
+			void __wrap_exit(int i) {
+				__real_exit(i);
+			}
+		]],
+		[[
+			exit(0);
+		]]
+	)],
+	[
+		AC_MSG_RESULT([yes])
+		have_ld_wrap_support=yes
+		AC_DEFINE([HAVE_LD_WRAP_SUPPORT], [1], [Define to 1 if linker supports -Wl,--wrap option])
+	],
+	[AC_MSG_RESULT([no])],
+)
+LDFLAGS="$saved_LDFLAGS"
+
 dnl We emulate signals in Windows
 AC_CHECK_DECLS(
 	[SIGHUP],

@@ -1244,6 +1269,7 @@ AM_CONDITIONAL([GIT_CHECKOUT], [test "${GIT_CHECKOUT}" = "yes"])
 AM_CONDITIONAL([ENABLE_PLUGIN_AUTH_PAM], [test "${enable_plugin_auth_pam}" = "yes"])
 AM_CONDITIONAL([ENABLE_PLUGIN_DOWN_ROOT], [test "${enable_plugin_down_root}" = "yes"])
 AM_CONDITIONAL([ENABLE_CRYPTO], [test "${enable_crypto}" = "yes"])
+AM_CONDITIONAL([HAVE_LD_WRAP_SUPPORT], [test "${have_ld_wrap_support}" = "yes"])
 
 plugindir="${with_plugindir}"
 sampledir="\$(docdir)/sample"
tests/unit_tests/openvpn/Makefile.am +5 -1
index fafe6b2..b902b20 100644
@@ -1,6 +1,10 @@
 AUTOMAKE_OPTIONS = foreign
 
-check_PROGRAMS = argv_testdriver buffer_testdriver
+check_PROGRAMS=
+
+if HAVE_LD_WRAP_SUPPORT
+check_PROGRAMS += argv_testdriver buffer_testdriver
+endif
 
 if ENABLE_CRYPTO
 check_PROGRAMS += tls_crypt_testdriver
Signature · 1 line
2.1.4
#1 lore raw

Hi,

Quoted 6 lines · On Wed, Jan 18, 2017 at 03:42:52PM -0500, [email protected]... wrote:

From: Selva Nair <[email protected]...>

- Also make tests that require --wrap option to be conditional on this support

Signed-off-by Selva Nair <[email protected]...>

I said so in the PR already, but for the sake of the list - I think this is a reasonable approach: disable the --wrap tests if we can't run them, but keep the rest of the cmocka stuff active (way better than my initial "if it's not GNU ld, just disable cmocka").

I'm ready and willing to ACK and merge this, but would like to hear some voices from folks that understand autoconf better - is there anything wrong with this approach? Any suggestions how to do it better?

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#2 lore raw
In reply to [email protected]...
Quoted 10 lines · On 18/01/17 21:42, [email protected]... wrote:

From: Selva Nair <[email protected]...>

- Also make tests that require --wrap option to be conditional on this support

Signed-off-by Selva Nair <[email protected]...>
---
 configure.ac                         | 26 ++++++++++++++++++++++++++
 tests/unit_tests/openvpn/Makefile.am |  6 +++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

This looks like a very good approach to me, and I believe it is done correctly. One comment though, below ...

Quoted 5 lines
configure.ac +0 -0
index 43487b0..0b7fea9 100644
@@ -582,6 +582,31 @@ AC_COMPILE_IFELSE(

[...snip...]

configure.ac AC_COMPILE_IFELSE( · line 604

Quoted 1 line
configure.ac +1 -0
+		AC_DEFINE([HAVE_LD_WRAP_SUPPORT], [1], [Define to 1 if linker supports -Wl,--wrap option])

Any reason to have this AC_DEFINE? That puts HAVE_LD_WRAP_SUPPORT into config.h, which I don't think makes much sense. If we don't have --wrap support, would we have #ifdef HAVE_LD_WRAP_SUPPORT in the C code at all?

[...snip...]

Quoted 1 line

+AM_CONDITIONAL([HAVE_LD_WRAP_SUPPORT], [test "${have_ld_wrap_support}" = "yes"])

This however is the clue of it all, to ensure we can do the proper check in Makefile.am files, as you've done below

configure.ac

Quoted 3 lines
configure.ac +3 -0
+if HAVE_LD_WRAP_SUPPORT
+check_PROGRAMS += argv_testdriver buffer_testdriver
+endif

It would be great if macOS/OSX users with LLVM compilers installed could run some tests with this patch by EOB tomorrow (Friday Jan 20).

If I don't hear any objects by then, I am going to give this an ACK without the AC_DEFINE line (unless good arguments having this in config.h surfaces).

Selva, if you don't mind ... I can use this patch and just take out the AC_DEFINE line at commit time.

Signature · 4 lines
kind regards,

David Sommerseth
OpenVPN Technologies, Inc
author
#3 lore raw

Hi,

Thanks for the comments.

Quoted 4 lines · David Sommerseth

Any reason to have this AC_DEFINE? That puts HAVE_LD_WRAP_SUPPORT into config.h, which I don't think makes much sense. If we don't have --wrap support, would we have #ifdef HAVE_LD_WRAP_SUPPORT in the C code at all?

Repeating from related discussion on the PR page: AC_DEFINE is indeed meant to add a preprocessor define (available to sources through config.h). We do not have a use for it right now but we could --- say one could provide an alternative to __wrap_xxx() in source, not sure how though.

So currently its just a preprocessor define that provides a solution waiting for a problem. If the consensus is not to define it, fine with me.

Selva

#4 lore raw
Quoted 13 lines · Selva Nair

Hi,

Thanks for the comments.

On Thu, Jan 19, 2017 at 9:41 AM, David Sommerseth
<[email protected]...
<mailto:[email protected]...>> wrote:

Any reason to have this AC_DEFINE? That puts HAVE_LD_WRAP_SUPPORT into config.h, which I don't think makes much sense. If we don't have --wrap support, would we have #ifdef HAVE_LD_WRAP_SUPPORT in the C code at all?

Quoted 5 lines

Repeating from related discussion on the PR page: AC_DEFINE is indeed meant to add a preprocessor define (available to sources through config.h). We do not have a use for it right now but we could --- say one could provide an alternative to __wrap_xxx() in source, not sure how though.

Exactly. There might be some possibilities, but I don't have a good idea currently. And I see even fewer possibilities where #ifdef around this would be a solution. I think a solution would require to compile and link a different set of source code providing a similar feature, which would anyway need to be primarily handled via configure.ac and Makefile.am.

And considering this is limited to the LLVM linker and works fine with at least the GNU linker, I don't think we will spend too much time tackling this now.

Quoted 2 lines

So currently its just a preprocessor define that provides a solution waiting for a problem. If the consensus is not to define it, fine with me.

I have no issues adding such an AC_DEFINE when we have a good use-case for it. But if we don't need it now, I think it's best to keep things as simple as possible, in particular in the autotools stuff which can already be quite confusing.

--
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

author
#5 lore raw

Hi,

Quoted 8 lines · David Sommerseth
Quoted 2 lines

So currently its just a preprocessor define that provides a solution waiting for a problem. If the consensus is not to define it, fine with

me.

I have no issues adding such an AC_DEFINE when we have a good use-case for it. But if we don't need it now, I think it's best to keep things as simple as possible, in particular in the autotools stuff which can already be quite confusing.

Then please take it out at merge time -- its only one line. And if we need it in future could be easily added.

Selva

#6 lore raw
In reply to [email protected]...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

ACK to this approach, with one commit-time modification (removed the AC_DEFINE line, as already discussed in the patch mail thread)

Your patch has been applied to the following branches

commit f91ab283a407e25c4b32aecb390911b212ce2694 (master) commit 2f5f1d8fffcba26d21d47cbcb1e99e0e1c313568 (release/2.4)

Author: Selva Nair
Date:   Wed Jan 18 15:42:52 2017 -0500
     Add a check for -Wl, --wrap support in linker

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Acked-by: David Sommerseth <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13897.html
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJYglP5AAoJEIbPlEyWcf3yDzMP/1LRWOqsbo2T0Wn7R4tgrEgB GsYNSgBMdIEzjcKCIgZPhVmqhNMTa9gNJYWpJ649KZM/sf5FrP/N4M0hNNoklpLc 6ScxPGnB0/iRGPlKFp+jPvK+WHpDKAidV0TQszHORB2O2BzY6W+hZ7UJIQ7Igb6P VN6vkzFq49jZiiiBbT9RQErp9o7Pv2h/+7gYoZlw0uhqgfGVzAbtJSQAk817C3y0 px/HUcTH14ANrIKfQNTryUQConMcmw6nJpGJFAP/yL2S8xfGgcmf7uNORprOBTle Anj5OvJuKRgenCQ03mYyiOLkq38aLBZnLmUBRnHayivwNoXnexBNOh7Xl8Pj1R9l RB/Vcx6cPa1FNLk4PpqFC5v9ZkBIDj9FWjsrbs7wlsbw7ud/KoSUgklqGNf18tZ7 bJZ4v0eLXmy1IxOVrmWNaFRn/NJJf4sCVoePmMovi0Exoydy3TRL5ZMTOgB3DeoM BmPow+StIUJYeG8ARfq9BHVhhDr4DP08aOLuECdNGKZexSK1jWZp6wqX7YmKQhdF OZRhHmxtzy6OzIu9n5XW9I9F1HZZIWWSDsLmhdWSIYCxaDis7AiUrKL04hTgrpoR 6aS06HRUxPGyP35NjwLcvyvpqOsuwlgCHFQ8FEfac0aGYCyCq3hK73III8dTfufw BW10WDHQUkUzubdzWqSz
=0Y0J
-----END PGP SIGNATURE-----

author
#7 lore raw
Quoted 5 lines · David Sommerseth

Your patch has been applied to the following branches

commit f91ab283a407e25c4b32aecb390911b212ce2694 (master) commit 2f5f1d8fffcba26d21d47cbcb1e99e0e1c313568 (release/2.4)

I am curious to know whether the opensolaris buildbot (there is one isn't it?) run and pass all tests?

Selva

#8 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 4 lines · Selva Nair

On Fri, Jan 20, 2017 at 1:16 PM, David Sommerseth <[email protected]... <mailto:[email protected]...>> wrote:

Quoted 8 lines

Your patch has been applied to the following branches

commit f91ab283a407e25c4b32aecb390911b212ce2694 (master) commit 2f5f1d8fffcba26d21d47cbcb1e99e0e1c313568 (release/2.4)

I am curious to know whether the opensolaris buildbot (there is one isn't it?) run and pass all tests?

We have a few opensolaris builds getting kicked off. If ignoring a temporary and odd t_client.sh error on one of the OSX builds, everything built correctly now without any issues.

TL;DR: Buildbot haven't had so many green cells as it has now in a long time. So I'm happy, and I believe Gert is happy too!

- --
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJYgnv6AAoJEIbPlEyWcf3yFJgP/163RFBSB5kNx4lr1+3CaNwz d/TDeX10/exjFZUZm0H3IbcN/VfFk+3IscIEOVOq0O4sQm9t6uRzP+zHKZhvulUd K98Lu/dBjZ86shrUmjelDJ6lo5grKcuPnOxkRrRA1GQY8CWoWdj+S2oyyT+I6Ji1 n710ddg8Z6QOSnajSLP6Mvj5kT4KVy2d7Wpg7rUyrtZINL7G/ntKO29ScdemWyTl EtdlXiCcXPzmsnIeP0wrGQDmCs5hehSHa7LgchwPL33HNdnKejaqAT7oyyZJtSX3 3CHxUntFE1dJ6jTArcO7eMbnxHvEX4yyIZ2fn+sMcXuCyACw7lpdFi47BCH4u1ou g0ulTQHi220O3ckjP2U9+wqzzZiSZLao6K8vra6r9PvLF/SeIF9l2bY1YS6riSBt W76zoMbI65Jdin8ooheODqxVLEDR7ctMiu6qO7jIGjlMC89t1pVb2yQ8EInl6EVi 0Zcl83n58irN2Z6eD7PDjThaHNYWn76daVW4cJuJvkLzNvX6DB0c0uOiNJkd4RRe 0ECjBi7ATlBy4L1yJoL86MYCr5qRPeEMRfakqP2jWwrX92JNF5tzllEJ9m7gLjyO dJaNvAtNu1uF3swHk/HDHRLi9vwz/92qbuje5ChUrM/XeLCxT6UouMr2IYaIzrYJ JUdSpsjnygKNaRYxXU3M
=7QMK
-----END PGP SIGNATURE-----

#9 lore raw
In reply to Selva Nair

Hi,

Quoted 2 lines · Selva Nair

I am curious to know whether the opensolaris buildbot (there is one isn't it?) run and pass all tests?

There is one, but it uses gcc and GNU ld, so --wrap never was a problem there...

[email protected]...:/home/gert$ ld --version
GNU ld version 2.15
Copyright 2002 Free Software Foundation, Inc.

I do not have a machine with the Sun development tools (yet, supposedly there is a way to get a free license, but I had no time to follow up on this)

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
author
#10 lore raw

Hi,

Quoted 10 lines · Gert Doering
Quoted 2 lines · Selva Nair

I am curious to know whether the opensolaris buildbot (there is one isn't it?) run and pass all tests?

There is one, but it uses gcc and GNU ld, so --wrap never was a problem there...

[email protected]...:/home/gert$ ld --version
GNU ld version 2.15
Copyright 2002 Free Software Foundation, Inc.

I see... Recently I've used opensolaris only as in Illumo's omnios and gcc -v shows

Target: i386-pc-solaris2.11
Configured with: ./configure --host i386-pc-solaris2.11 [...snip...] --without-gnu-ld --with-ld=/bin/ld --with-as=/usr/bin/gas --with-gnu-as
etc..

And /bin/ld gives
usage: ld [-3:6:abc:d:e:f:h:il:mo:p:rstu:z:B:CD:F:GI:L:M:N:P:Q:R:S:VW:Y:?] file(s)

It does support --wrap though.

Selva