849093 - autoconf: Do not call AC_PROG_CC inside a conditional block

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Wan-Teh Chang]

Attachment: The location of the problem and a hack

This is the hardest problem I ran into when trying to
make NSPR's configure.in work with the current version
of autoconf.

The attached patch shows where the problematic AC_TRY_COMPILE
is, and includes a hack that solves the problem.

Without this patch, NSPR's configure script generated by
the current version of autoconf cannot test-compile C code.
I tracked it down to ac_objext (object file extension) being
empty, so the configure script cannot find the object file
"conftest.$ac_objext" (which expands to "conftest.") because
the correct object file name is "conftest.o".

After a lot of debugging, I found that this is because the
code that sets ac_objext (you can find it by searching for
"ac_objext=") is inside the conditional block for cross
compiling, so the non-cross compiling case does not execute
that code, and therefore ac_objext is not set.

The code that sets ac_objext is the autoconf internal macro
_AC_COMPILER_OBJEXT. autoconf uses m4_expand_once with the
_AC_COMPILER_OBJEXT macro, as part of AC_PROG_CC.

I think this means _AC_COMPILER_OBJEXT is expanded only once,
the first time AC_PROG_CC is used. Apparently AC_TRY_COMPILE
uses or requires AC_PROG_CC. I did not fully track that down,
but using the "wtc point 1" and "wtc point 2" comments I added
to configure.in, I narrowed down the culprit to be that
AC_TRY_COMPILE in the conditional block for cross compiling.

My hack is to add an extra AC_PROG_CC macro call outside the
conditional block. I don't know if that will break cross
compiling.

Comment 1

[Wan-Teh Chang]

I think this bug is the reason for the last two changes in this patch:
http://hg.et.redhat.com/cgi-bin/hg-misc.cgi/fedora-mingw--devel/annotate/c376c13886ec/nspr/nspr-configure-remove-crack.patch

This problem prevents us from checking whether the compiler is GCC
or checking for a header.

Comment 2

[Mike Frysinger]

i don't quite understand what you're trying to do here.  why can't you just call AC_PROG_CC at the top of the configure file (near where you call AC_CANONICAL_SYSTEM) ?  this is how every other autoconf project out there works.

that macro handles cross-compiling just fine w/out any additional magic on your part.

Comment 3

[Wan-Teh Chang]

vapier: I can explain two things we are doing differently. Unfortunately
I am not familiar with how NSPR's configure.in handles cross compiling,
so I can't explain that part of the file.

1. We must call AC_PROG_CC after setting our own default value of CC:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/configure.in&rev=1.341&mark=458-491#458

The NSPR team used the compilers from the Unix system vendors by
default. I believe AC_PROG_CC prefers GCC if 'CC' is not specified.
So we have to call AC_PROG_CC after setting our own default value of CC.

2. If we are using MSVC on Windows:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/configure.in&rev=1.341&mark=68,77,86,89,93-97#68

we do not call AC_PROG_CC:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/configure.in&rev=1.341&mark=562,629,654,819#562

I believe this was caused by a lack of good Win32 support of an older
version of autoconf (2.12?) or a limitation of the MKS Korn Shell
that we were using.

It is also possible that since we know the features supported by
Windows, we can hardcode the results of the feature tests without
performing the feature tests.

3. What I don't know well about is the cross compiling section:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/configure.in&rev=1.341&mark=567-652#562

Unfortunately this is the part that's causing problem with
setting ac_objext under autoconf 2.50+, and why I am having
a hard time coming up with a fix.

Comment 4

[Mike Frysinger]

(In reply to Wan-Teh Chang from comment #3)

(1) why do we care anymore ?  why do we want to let targets that are (arguably) fringe systems hold us back ?

if you want to use a different compiler with autotools, there is a well established standard here: set CC yourself.
  CC=<some weird compiler> ./configure

the current logic in autotools is that it does first look for `gcc` and if that isn't found, it falls back to `cc` (and `cl.exe` after that).

(2) the AC_PROG_CC code supports looking for `cl.exe` now.

(3) i don't think this logic is largely necessary anymore.  latest autoconf sets up a variable ($cross_compiling) for you now that you can check to see if you are cross-compiling.  it also takes care of searching for ${ac_tool_prefix}cc by default so that something like `./configure --host=i386-linux-gnu` automatically finds `i386-linux-gnu-gcc`.

btw, that logic is largely wrong -- i'm pretty sure nspr shouldn't be using $target anywhere.  $build and $host are the only fields it should care about.

the rest of those variable setups (RANLIB/AR/AS/etc...) either have macros readily available in the latest autoconf, or they're easy to implement:
  AC_CHECK_TOOL(RANLIB, ranlib)
this will handle searching for the cross-compiler prefixed name first.

iirc, nspr does want both a build compiler and a host compiler because it compiles & runs some tools (like nsinstall?) at build time.  that will require a little bit of new logic, but that should be easy to pull off.

i can help gut this cross-compiling logic because all i do every day is cross-compile :).

Comment 5

[Wan-Teh Chang]

Attachment: A brute-force fix

I don't understand cross-compilation well enough to come up with
an elegant fix, so I am using brute force.

The requirement is that AC_PROG_CC must be invoked outside any
conditional blocks, and before any macros that may use the compiler,
such as AC_TRY_COMPILE. Here is my solution:

1. The two AC_PROG_CC calls are inside the two conditional blocks
of an if-else statement. Break the if-else statement into a
"before AC_PROG_CC" part and an "after AC_PROG_CC" part, so that
we call AC_PROG_CC, only once, outside any conditional block.

2. Move the first AC_TRY_COMPILE call after the AC_PROG_CC call.

Comment 6

[Wan-Teh Chang]

Comment on attachment 8390243 [details] [diff] [review]
A brute-force fix

Review of attachment 8390243 [details] [diff] [review]:
-----------------------------------------------------------------

I annotated my patch to help you review it.

::: configure.in
@@ +605,5 @@
> +
> +    AC_CHECK_PROGS(CC, $CC "${target_alias}-gcc" "${target}-gcc", echo)
> +    unset ac_cv_prog_CC
> +    dnl Now exit the conditional block to invoke AC_PROG_CC.
> +fi

Here is the end of the "before AC_PROG_CC" part in the new code.

@@ +611,5 @@
> +dnl In the latest versions of autoconf, AC_PROG_CC is a one-shot macro,
> +dnl declared with AC_DEFUN_ONCE. So it must not be expanded inside a
> +dnl conditional block. Invoke AC_PROG_CC outside any conditional block
> +dnl and before invoking AC_TRY_COMPILE (which requires AC_PROG_CC).
> +AC_PROG_CC

Here is the only AC_PROG_CC call in the new code.

@@ +614,5 @@
> +dnl and before invoking AC_TRY_COMPILE (which requires AC_PROG_CC).
> +AC_PROG_CC
> +
> +dnl Reenter the conditional blocks after invoking AC_PROG_CC.
> +if test "$target" != "$host" -o -n "$CROSS_COMPILE"; then

Here is the beginning of the "after AC_PROG_CC" part in the new code.

@@ -605,1 @@
>      AC_TRY_COMPILE([], [return 0;],

Here is the first AC_TRY_COMPILE call in configure.in. I will move
the code that calls AC_PROG_CC before this AC_TRY_COMPILE call.

@@ -634,5 @@
> -    esac
> -
> -    AC_CHECK_PROGS(CC, $CC "${target_alias}-gcc" "${target}-gcc", echo)
> -    unset ac_cv_prog_CC
> -    AC_PROG_CC

Here is the first AC_PROG_CC call in the original call. It is in
the middle of the "if" block.

@@ -662,2 @@
>  else
> -    AC_PROG_CC

Here is the second AC_PROG_CC call in the original call. Note that
it is at the very beginning of the "else" block.

Comment 7

[Wan-Teh Chang]

Comment on attachment 8390243 [details] [diff] [review]
A brute-force fix

Review of attachment 8390243 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +657,1 @@
>      AC_TRY_COMPILE([], [return 0;],

Here is the first AC_TRY_COMPILE call in the new code.

Comment 8

[Wan-Teh Chang]

Comment on attachment 8390243 [details] [diff] [review]
A brute-force fix

This patch broke cross-compilation. Back to the drawing board...

Comment 9

[Wan-Teh Chang]

Comment on attachment 8390243 [details] [diff] [review]
A brute-force fix

I found that the Mozilla try builds were not broken by this patch. So
this patch is ready for review.

Details:

If I run autoconf-2.13 to generate nsprpub/configure, then this patch
alone is enough.

If I run autoconf-2.68 to generate nsprpub/configure, then I also
need to change mozilla/configure.in. The source of the problem is that
autoconf-2.68 performs sanity checks on the shared config.cache file,
and it doesn't allow the "precious" variables such as CFLAGS and
LDFLAGS to differ. Since mozilla/configure.in adds some flags to
the CFLAGS and LDFLAGS passed to nsprpub/configure, such differences
result in errors. So Mozilla and NSPR cannot share a config.cache
file, and mozilla/configure.in needs to invoke nsprpub/configure
in a way similar to how it invokes libffi's configure script.

Comment 10

[Benjamin Smedberg]

Comment on attachment 8390243 [details] [diff] [review]
A brute-force fix

I don't have an opinion about this, Ted's review should be sufficient.

Comment 11

[Wan-Teh Chang]

Comment on attachment 8390243 [details] [diff] [review]
A brute-force fix

Review of attachment 8390243 [details] [diff] [review]:
-----------------------------------------------------------------

Kai, could you also review this patch?

If you are unfamiliar with autoconf, just verify that two code transformations
I performed result in equivalent code.

Transformation 1: Move the block of code that checks the $host compiler $HOST_CC
after the block of code that checks the compiler $CC.

Note: you may not know whether these two blocks of code need to be executed in
a particular order. So, please assume that the order of these two blocks of code
doesn't matter.

Transformation 2: pull the AC_PROG_CC statement, common to the "if" and "else"
blocks, outside the if-else statement. This requires breaking the if-else
statement into a "before" part and an "after" part.

I have tested this patch on the Mozilla try server. It doesn't break any try
server builds.

Thanks.

Comment 12

[Kai Engert (:KaiE:)]

Comment on attachment 8390243 [details] [diff] [review]
A brute-force fix

Sorry, I don't have time to carefully review this.

I trust that you have sufficiently tested it, and therefore give you a rubberstamp r=kaie

Comment 13

[Kai Engert (:KaiE:)]

Comment on attachment 8390243 [details] [diff] [review]
A brute-force fix

sorry, I accidentally removed Wan-Teh's request for Ted's review

Comment 14

[Wan-Teh Chang]

Attachment: A brute-force fix, v2

I moved the code that checks HOST_CC and does a try compile
with it after all the target tool checks. I think the code
reads better that way. I also removed some spaces at the end
of lines.

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/3e9df6c69329

Comment 15

[Wan-Teh Chang]

Comment on attachment 8394840 [details] [diff] [review]
A brute-force fix, v2

Patch pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06d06dcf812c

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/06d06dcf812c

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 8394840 [details] [diff] [review]
A brute-force fix, v2

Review of attachment 8394840 [details] [diff] [review]:
-----------------------------------------------------------------

This is kind of terrible, but everything about autoconf is terrible in general.