bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: expand-before-require bug


From: Eric Blake
Subject: Re: expand-before-require bug
Date: Thu, 22 Jan 2009 06:40:55 -0700
User-agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.19) Gecko/20081209 Thunderbird/2.0.0.19 Mnenhy/0.7.5.666

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

According to Bruno Haible on 1/22/2009 5:31 AM:
> Eric,

>> * modules/errno (configure.ac): Require, rather than expand,
>> gl_HEADER_ERRNO_H.
>> * m4/errno_h.m4 (gl_HEADER_ERRNO_H_BODY): Merge...
>> (gl_HEADER_ERRNO_H): ...into this macro, and use AC_DEFUN_ONCE to
>> enforce that all clients require it.
>
> This change is a step backwards. It makes the problem worse.

I tend to disagree; reasoning below.  At any rate, I don't see any reason
to revert the gnulib patches unless/until autoconf does not warn on the
cases that those patches resolved.

> 
> So my and your workaround (in gl_HEADER_ERRNO_H and gl_MULTIARCH) was to
> write and publish macros for which, when they are both invoked and
> required, there is *no* problem. This is a good step because ultimately
> it eases maintenance if we don't have to remember "this macro must only
> be invoked" and "that macro must only be required".
> 
> Your two patches now reintroduce the problem. But no, I don't want to
> think about AC_REQUIRE's bug every time I want to invoke an autoconf macro!

My argument is that if Autoconf can enforce rules, such as refusing to
compile if you try to expand instead of require an AC_DEFUN_ONCE macro,
then it is easy to fix your code if you choose the wrong method.
Requiring then expanding for AC_DEFUN is generically ok (although you get
redundant expansion, so the output is larger); the problem is only with
expanding then requiring.

> The question what to do with the warning then is of secondary nature.
You can
>   - make it a non-default warning (like -Wshadow in gcc - not part of
- -Wall), or

No; the warning catches _actual_ bugs.  For example, the gnulib bug with
AC_GNU_SOURCE before AC_USE_SYSTEM_EXTENSIONS really did result in
out-of-order expansion, as evidenced by the fact that after the patch, the
expansion of AC_USE_SYSTEM_EXTENSIONS occurs earlier in the generated
configure file.

>   - fix the warning to apply only to AC_DEFUN_ONCEd macros. After all, I
used
>     AC_DEFUN, not AC_DEFUN_ONCE, to define gl_HEADER_ERRNO_H. Then why
do you
>     emit a warning about it at all?

Because the user's bug of expand-before-require affects both AC_DEFUN'd
and AC_DEFUN_ONCE'd macros.  The only thing that using AC_DEFUN_ONCE
instead of AC_DEFUN adds is that you get an automatic warning, even with
older autoconf, that a macro was expanded more than once.

>   - fix the warning to recognize macros which, other than for AC_REQUIRE
>     invocations, produce only whitespace output, or (better)

It is next to impossible to efficiently figure out whether an m4 macro
body consists of only whitespace and AC_REQUIRE.

> 
> I wrote:
>>   - provide a documented way to avoid the warning for a particular macro,
>>     I much prefer to have a macro which can be both invoked and required,
>>     even if it requires one more line of code to write it down, than to have
>>     a macro with restricted use.
> 
> Here's a concrete proposal: Introduce a new macro-defining-macro
> AC_DEFUN_IDEMPOTENT, such that:
> 
>   - macros defined with AC_DEFUN_IDEMPOTENT are *known* to be expandable
>     any number of times, hence they may be both invoked and required,

I suppose I could implement this for 2.64:

m4_define([AC_DEFUN_IDEMPOTENT],
[m4_set_add([_m4_idempotent], [$1])AC_DEFUN([$1], [$2])])

then make the warning conditional on whether the macro is a member of the
set.  But this definition won't port back to 2.63 or earlier, so gnulib
would have to do something like this fallback:

m4_ifndef([AC_DEFUN_IDEMPOTENT],
[m4_define([AC_DEFUN_IDEMPOTENT], m4_defn([AC_DEFUN]))])

Meanwhile, you still have the scenario where a macro declared idempotent
may result in duplicated output when using 2.64 (okay, since you declared
it was idempotent), but silent out-of-order expansion when using 2.63 or
earlier.  Which means the onus is thus on the developer using
AC_DEFUN_IDEMPOTENT to ensure that their macro consists only of whitespace
and AC_REQUIRE, if they don't want to be impacted by expand-before-require
bugs.  But autoconf can't enforce that a macro declared idempotent was
well-written.

> 
>   - macros defined with AC_DEFUN_ONCE are *known* to be expandable only
>     once, hence the recommendation for them is to AC_REQUIRE them,

That is already the case, although I can further improve it in 2.64, by
issuing a warning if such a macro was not invoked via AC_REQUIRE or
expanded at the top level.  But again, this improved warning is only
available if you develop with autoconf 2.64, so it is still possible that
developers using older autoconf will introduce bugs.

> 
>   - about macros defined with AC_DEFUN nothing is known.
> 
> So for your warning about required-after-expanded it means:
>   - the warning should apply always to AC_DEFUN_ONCEd macros,

To be clear, you are proposing multiple warnings.  In this case, one
warning is attached to any direct expansion of a defun_once macro inside a
defun macro body, regardless of any other requires that have taken place:

`macro' should be AC_REQUIRE'd inside AC_DEFUN

(hmm, that gets tricky - there is currently no way to know whether the
macro was declared via m4_defun or its synonym AC_DEFUN; with
m4_require/AC_REQUIRE the solution is to look at $0, but with direct
invocation of an AC_DEFUN_ONCE macro, we don't have context to make the
warning message nice)

then, since we already warned about direct expansion, any instance of an
expand-before-require warning becomes noise for a defun_once macro.

>   - the warning should never apply to AC_DEFUN_IDEMPOTENTd macros,

possible, but puts the onus on the user to use this style of macro
definition wisely

>   - the warning should apply to AC_DEFUNd macros only when the warning
>     level is higher than the default. The warning should indicate that
>     either the double expansion is a problem, or the macro should be
>     changed to be defined with AC_DEFUN_IDEMPOTENT if it is not a problem.

Here, the warning should always be issued.  If double-expansion was not a
problem, then AC_DEFUN_IDEMPOTENT will silence the warning.  And if
double-expansion IS a problem under 2.64, then you have a case where the
code causes silent out-of-order expansion with earlier autoconf, in which
case the warning is the right thing to do because it is a true bug in the
user's macros.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkl4d2cACgkQ84KuGfSFAYCO8gCfT+ZgbY54Z41hLC24iIxp92vR
vFcAoL7mWChnHHoWpP/J0rNLRloZkW/N
=4A0o
-----END PGP SIGNATURE-----




reply via email to

[Prev in Thread] Current Thread [Next in Thread]