[11/13] Fix va_start related location

In gcc/testsuite/gcc.dg/pr30457.c, the first warning was not being
emitted because the relevant location was inside the var_start macro
defined in a system header.  It can even point to a token for a
builtin macro there.  This patch unwinds to the first token in real
source code in that case.

Tested on x86_64-unknown-linux-gnu against trunk.

	* builtins.c (fold_builtin_next_arg): Unwinds to the first
	location in real source code.
---
 gcc/builtins.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

On Wed, Apr 25, 2012 at 9:04 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> In gcc/testsuite/gcc.dg/pr30457.c, the first warning was not being
> emitted because the relevant location was inside the var_start macro
> defined in a system header.  It can even point to a token for a
> builtin macro there.  This patch unwinds to the first token in real
> source code in that case.

While you are at it, could you also use a non-zero value for the second
argument argument to warning_at?

>
> Tested on x86_64-unknown-linux-gnu against trunk.
>
>        * builtins.c (fold_builtin_next_arg): Unwinds to the first
>        location in real source code.
> ---
>  gcc/builtins.c |   16 ++++++++++++++--
>  1 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index b47f218..ef90b25 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -12164,8 +12164,20 @@ fold_builtin_next_arg (tree exp, bool va_start_p)
>          the default argument promotions, the behavior is undefined."
>       */
>       else if (DECL_REGISTER (arg))
> -        warning (0, "undefined behaviour when second parameter of "
> -                 "%<va_start%> is declared with %<register%> storage");
> +       {
> +         /* There is good chance the current input_location points
> +            inside the definition of the va_start macro (perhaps on
> +            the token for builtin) in a system header, so the warning
> +            will not be emitted.  Use the location in real source
> +            code.  */
> +         source_location current_location =
> +           linemap_unwind_to_first_non_reserved_loc (line_table, input_location,
> +                                                     NULL);
> +         warning_at (current_location,
> +                     0,
> +                     "undefined behaviour when second parameter of "
> +                     "%<va_start%> is declared with %<register%> storage");
> +       }
>
>       /* We want to verify the second parameter just once before the tree
>         optimizers are run and then avoid keeping it in the tree,
> --
>                Dodji

Gabriel Dos Reis <gdr@integrable-solutions.net> writes:

> On Wed, Apr 25, 2012 at 9:04 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>> In gcc/testsuite/gcc.dg/pr30457.c, the first warning was not being
>> emitted because the relevant location was inside the var_start macro
>> defined in a system header.  It can even point to a token for a
>> builtin macro there.  This patch unwinds to the first token in real
>> source code in that case.
>
> While you are at it, could you also use a non-zero value for the second
> argument argument to warning_at?

I couldn't find any obvious value for it.  I am thinking maybe it would
make sense to introduction a new -Wva_start to warn about possible
dangerous uses of the va_start macro and use that as the second argument
for the relevant warnings emitted by fold_builtin_next_arg.  What do you
think?

In any case, this topic seems logically unrelated to the purpose of
enable -ftrack-macro-expansion by default, so IMHO it would be better to
do this in a separate self contain patch.  Among other things, this
would ease possible future back-ports.  Would you agree?

Thanks.

On Wed, Apr 25, 2012 at 10:20 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> Gabriel Dos Reis <gdr@integrable-solutions.net> writes:
>
>> On Wed, Apr 25, 2012 at 9:04 AM, Dodji Seketeli <dodji@redhat.com> wrote:
>>> In gcc/testsuite/gcc.dg/pr30457.c, the first warning was not being
>>> emitted because the relevant location was inside the var_start macro
>>> defined in a system header.  It can even point to a token for a
>>> builtin macro there.  This patch unwinds to the first token in real
>>> source code in that case.
>>
>> While you are at it, could you also use a non-zero value for the second
>> argument argument to warning_at?
>
> I couldn't find any obvious value for it.  I am thinking maybe it would
> make sense to introduction a new -Wva_start to warn about possible
> dangerous uses of the va_start macro and use that as the second argument
> for the relevant warnings emitted by fold_builtin_next_arg.  What do you
> think?

or -Wvarargs?


>
> In any case, this topic seems logically unrelated to the purpose of
> enable -ftrack-macro-expansion by default, so IMHO it would be better to
> do this in a separate self contain patch.  Among other things, this
> would ease possible future back-ports.  Would you agree?

OK.

-- Gaby