[RFC] configure: Make sure config.h isn't accidentally included in builds for the host

Message ID 1478176330-2885-1-git-send-email-martin@martin.st
State New
Headers show

Commit Message

Martin Storsjö Nov. 3, 2016, 12:32 p.m.
In aacps_tablegen.h, only include libm.h if building for the target.

If actual fallbacks are needed here (in practice, sinf and cosf,
which are missing on Plan 9 - they are present even on MSVC), we need
to include libm.h, but this relies on configure test results for the
target. These test results can't be used for the host (e.g. when this
header is used when building with --enable-hardcoded-tables).

This clearly breaks builds with hardcoded tables on Plan 9 (not
cross builds from another host to Plan 9 though), instead of relying on
coincidences that config.h for the target might match the host build
config as well.
---
 configure                   | 4 ++++
 libavcodec/aacps_tablegen.h | 2 ++
 2 files changed, 6 insertions(+)

Comments

Diego Biurrun Nov. 3, 2016, 7:34 p.m. | #1
On Thu, Nov 03, 2016 at 02:32:10PM +0200, Martin Storsjö wrote:
> In aacps_tablegen.h, only include libm.h if building for the target.
> 
> If actual fallbacks are needed here (in practice, sinf and cosf,
> which are missing on Plan 9 - they are present even on MSVC), we need
> to include libm.h, but this relies on configure test results for the
> target. These test results can't be used for the host (e.g. when this
> header is used when building with --enable-hardcoded-tables).
> 
> This clearly breaks builds with hardcoded tables on Plan 9 (not
> cross builds from another host to Plan 9 though), instead of relying on
> coincidences that config.h for the target might match the host build
> config as well.

So, speaking of Plan 9, IMO we should discuss if keeping support for it
is worth the trouble. It was a nice joke and pun with the 9 release, but
that has run its course.

> --- a/configure
> +++ b/configure
> @@ -3719,6 +3719,7 @@ check_cc -D_LARGEFILE_SOURCE <<EOF && add_cppflags -D_LARGEFILE_SOURCE
>  EOF
>  
>  add_host_cppflags -D_ISOC99_SOURCE
> +add_host_cppflags -DHOST_BUILD

You can merge these two lines.

>  check_host_cflags -std=c99
>  check_host_cflags -Wall
>  check_host_cflags -O3
> @@ -5356,6 +5357,9 @@ cat > $TMPH <<EOF
>  #define EXTERN_PREFIX "${extern_prefix}"
>  #define EXTERN_ASM ${extern_prefix}
>  #define SLIBSUF "$SLIBSUF"
> +#ifdef HOST_BUILD
> +#error config.h must not be included in host builds
> +#endif
>  EOF

Go figure, I had practically the same idea some time ago ..

Diego
Martin Storsjö Nov. 3, 2016, 8:25 p.m. | #2
On Thu, 3 Nov 2016, Diego Biurrun wrote:

> On Thu, Nov 03, 2016 at 02:32:10PM +0200, Martin Storsjö wrote:
>> In aacps_tablegen.h, only include libm.h if building for the target.
>> 
>> If actual fallbacks are needed here (in practice, sinf and cosf,
>> which are missing on Plan 9 - they are present even on MSVC), we need
>> to include libm.h, but this relies on configure test results for the
>> target. These test results can't be used for the host (e.g. when this
>> header is used when building with --enable-hardcoded-tables).
>> 
>> This clearly breaks builds with hardcoded tables on Plan 9 (not
>> cross builds from another host to Plan 9 though), instead of relying on
>> coincidences that config.h for the target might match the host build
>> config as well.
>
> So, speaking of Plan 9, IMO we should discuss if keeping support for it
> is worth the trouble. It was a nice joke and pun with the 9 release, but
> that has run its course.

Well, keeping it in itself isn't too much trouble, except we have no idea 
if it still works, and for things like this particular include of libm.h, 
I can't test. That said, I'm not opposed to removing it either, bringing 
the set of things back to things we test regularly.

>> --- a/configure
>> +++ b/configure
>> @@ -3719,6 +3719,7 @@ check_cc -D_LARGEFILE_SOURCE <<EOF && add_cppflags -D_LARGEFILE_SOURCE
>>  EOF
>>
>>  add_host_cppflags -D_ISOC99_SOURCE
>> +add_host_cppflags -DHOST_BUILD
>
> You can merge these two lines.

Ah, yes

// Martin

Patch

diff --git a/configure b/configure
index 9264bb5..b43642f 100755
--- a/configure
+++ b/configure
@@ -3719,6 +3719,7 @@  check_cc -D_LARGEFILE_SOURCE <<EOF && add_cppflags -D_LARGEFILE_SOURCE
 EOF
 
 add_host_cppflags -D_ISOC99_SOURCE
+add_host_cppflags -DHOST_BUILD
 check_host_cflags -std=c99
 check_host_cflags -Wall
 check_host_cflags -O3
@@ -5356,6 +5357,9 @@  cat > $TMPH <<EOF
 #define EXTERN_PREFIX "${extern_prefix}"
 #define EXTERN_ASM ${extern_prefix}
 #define SLIBSUF "$SLIBSUF"
+#ifdef HOST_BUILD
+#error config.h must not be included in host builds
+#endif
 EOF
 
 test -n "$malloc_prefix" &&
diff --git a/libavcodec/aacps_tablegen.h b/libavcodec/aacps_tablegen.h
index a53f9fa..c075681 100644
--- a/libavcodec/aacps_tablegen.h
+++ b/libavcodec/aacps_tablegen.h
@@ -32,7 +32,9 @@ 
 #include "libavcodec/aacps_tables.h"
 #else
 #include "libavutil/common.h"
+#ifndef HOST_BUILD
 #include "libavutil/libm.h"
+#endif
 #include "libavutil/mathematics.h"
 #include "libavutil/mem.h"
 #define NR_ALLPASS_BANDS20 30