[FFmpeg-devel] [PATCH] libavcodec/wmavoice.c: localvariablepowmight mask pow function

[Måns Rullgård]

"Axel Holzinger" <aholzinger at gmx.de> writes:

> On Mon, Jul 26, 2010 at 12:56 AM, M?ns Rullg?rd wrote:
>> "Axel Holzinger" <aholzinger at gmx.de> writes:
>> 
>> > On Mon, Jul 26, 2010 at 11:18 AM, Eli Friedman wrote: 
>> >> On Mon, Jul 26, 2010 at 1:27 AM, Axel Holzinger <aholzinger at gmx.de> 
>> >> wrote:
>> >> > Hi Eli et al,
>> >> >
>> >> > On Sun, Jul 25, 2010 at 9:41 PM, Eli Friedman wrote:
>> >> >> On Sun, Jul 25, 2010 at 12:18 PM, Axel Holzinger 
>> >> >> <aholzinger at gmx.de> wrote:
>> >> >> > The issue is that a local variable pow masks a 
>> >> >> >function pow and
>> >
>> >> >> > that hurts if powf - which is used in the scope of local 
>> >> >> > variable pow - is not a function but a macro using double 
>> >> >> > precision pow function.
>> >> >>
>> >> >> And I'm pointing out that such an implementation is not correct 
>> >> >> per C99.
>> >> >
>> >> > I read the standard and under 7.1.4 (1) it's stated that 
>> >> > standard headers may export function like macros. I don't see
>> >> > how this couldn't be correct C99. Could you please provide a 
>> >> > reference where it's stated that a function like macro is
>> >> > forbidden in a standard header (at least for powf)?
>> >> 
>> >> It's okay for powf to be a macro.
>> >> 
>> >> > Also Unix/Posix allows powf to be implemented as a macro:
>> >> > http://www.opengroup.org/onlinepubs/009695399/basedefs/math.h.html
>> >> >
>> >> >> > Besides taking the effort to apply and document a patch
>> >> >> > patch, what do you think stands against the patch?
>> >> >>
>> >> >> Simply that you haven't actually pointed to why this is 
>> >> >> necessary
>> >
>> >> >> other than the abstract possibility of a non-conforming 
>> >> >> implementation of math.h.
>> >> >
>> >> > As mentioned above, I can't see why such an 
>> >> > implementation would be non-conforming. Can you elaborate.
>> >> 
>> >> Per C99, 7.1.3p1 and p2, "pow" is only reserved for use as a macro 
>> >> name or a file-scope identifier.  Therefore, it's legal to 
>> >> use it as the name of a local variable in any context.  A specific 
>> >> case of this is that the construct in wmavoice.c is legal C99 code,
>> >> and is required to compile on any conforming C99 implementation.
>> >
>> > Perhaps you don't yet understand the problem. Yes, it is 
>> > legal to use pow as a local variable name. Yes, it is also legal that
>> > powf is defined in <math.h> as a macro. That does not mean that the
>> > code in wmavoice.c is required to compile. This is one of the more
>> > obscure and ugly consequences of macros.
>> >
>> > Specifically, if powf() is defined as a function-like macro in 
>> > <math.h>, its definition would most likely use the pow() 
>> > function. You can expect something like the following:
>> >
>> > #define powf(a,b) ((float)pow((double)(a),(double)(b)))
>> >
>> > It is entirely legal C99 to have this macro definition in <math.h>,
>> 
>> No, it is not.  Since 'pow' is not reserved for identifers 
>> with local scope, the header may not use it in any way which 
>> might be incompatible with such use of the identifier in 
>> application code.  If math.h were to implement powf() as a 
>> macro, it would have to define
>> __pow() as an alias for pow() and use this in the macro definition.
>> Identifiers starting with __ (double underscore) are reserved 
>> for any use, so this would not cause problems for any 
>> conforming application code.
>
> Fair enough, could you please point me to clause in the standard
> that documents this?

As Eli already pointed out, 7.1.3 would be a good place to start.

>> > and the consequence in wmavoice.c is that the local variable pow 
>> > prevents the function pow() from being visible to the powf() macro, 
>> > leading to a compilation error.
>> 
>> Did this actually happen to you, or are you simply trolling?
>
> As you may have already have noticed from other threads, I'm
> currently trying to build FFmpeg on MinGW32 with icl (Intel compiler
> for windows) as compiler. As Intel doesn't bring a complete library,
> but instead fall back on some of the MS headers, math.h of MS is
> used.
>
> In the x64 part powf is a function and in the x86 part powf is a macro
> using the double precision version pow.

Then those headers are not conforming to the standard.

> Now, if you are right that would be illegal for a standard lib
> header (not only for C99, but also for C89, which MS claims to be
> compatible with). Still they do - and belieive me I know your
> reluctance regarding MS - the patch doesn't add any code cluttering
> or #ifdef orgy, but solves this issue and potentially others.

It solves a non-issue.  If it becomes an actual issue, we will fix it.

>> > In general, if you use a function-like macro, you have to take care 
>> > not to redefine any of the identifiers that it uses in its definition 
>> > (unless you know exactly what you're doing). As the standard does not 
>> > specify what those identifiers are, you are essentially stuffed. In 
>> > practice, you can apply common sense and avoid using those identifiers 
>> > which are most likely to create trouble.
>> 
>> The standard _does_ reserve the __* namespace for 
>> implementation use, so those are the names such macros must 
>> use.  It is perfectly safe and predictable.
>
> I just looked at MinGW's math.h:
> #define isfinite(x) ((fpclassify(x) & FP_NAN) == 0)
>
> This would then also be illegal, because the usage of fpclassify would
> mask a local variable fpclassify.
>
> On my Kubuntu 9.04 in math.h I find
>
> #  define isgreater(x, y) \
>   (__extension__
>    ({ __typeof__(x) __x = (x); __typeof(y) __y = (y);
>       !isunordered (__x, __y) && __x > __y; }))
>
> This masks a local variable isunordered.

fpclassify() and isunordered() are standard macros defined in math.h,
making them reserved if that header is included.  The macros above are
thus perfectly legit.

-- 
M?ns Rullg?rd
mans at mansr.com