document that alias and target must have the same type

GCC diagnoses declarations of function aliases whose type doesn't
match that of the target (ditto for attribute weakref).  It doesn't
yet diagnose such incompatbilities for variable aliases but that's
just an oversight that I will try to remember to correct in GCC 11.
The attached patch updates the manual to make it clear that
aliases must have the same type as their targets, or the behavior
is undefined (and may be diagnosed).

Martin

On 2/4/20 6:05 PM, Martin Sebor wrote:
> GCC diagnoses declarations of function aliases whose type doesn't
> match that of the target (ditto for attribute weakref).  It doesn't
> yet diagnose such incompatbilities for variable aliases but that's
> just an oversight that I will try to remember to correct in GCC 11.
> The attached patch updates the manual to make it clear that
> aliases must have the same type as their targets, or the behavior
> is undefined (and may be diagnosed).

On further review I noticed a few problems with the documentation
of attribute weakref.  The manual says that

   Without a target, given as an argument to weakref or to alias,
   weakref is equivalent to weak.

and

   At present, a declaration to which weakref is attached can only
   be static.

However, GCC accepts the following declaration:

   extern int x (void) __attribute__ ((weakref));

so the second sentence isn't correct without qualification (unlike
weakref, a weak declaration must be external).

Another documentation problem is with the example in the manual that
says that this declaration:

   static int x() __attribute__ ((weakref ("y")));

   /* is equivalent to... */

   static int x() __attribute__ ((weak, weakref, alias ("y")));

but GCC rejects the latter with

   error: weak declaration of ‘x’ must be public

Changing the latter from static to extern changes the error to

   error: ‘weakref’ symbol ‘x’ must have static linkage

So clearly two declarations with the two sets of attributes are not
equivalent, either extern or static.

I've fixed up these documentation problems in the attached revision
of the original patch.  I also mention that besides their types
having to match, the alias must have the same size and alignment
as the target.

Martin

PS I also noted that when the function is then also used GCC issues:

   warning: ‘weakref’ attribute should be accompanied with an ‘alias’ 
attribute

This matches what the manual says in "Without arguments, it should
be accompanied by an alias attribute naming the target symbol."

But when the weakref function is not used there is no warning.
That seems like an unfortunate side-effect of the choice of issuing
the warning a little too late (waning from the front-end it would
make it consistent regardless of whether the function was used, and
it would highlight the omission even in translation units that
define the weakref without using it).

Ping: https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00290.html

On 2/5/20 1:13 PM, Martin Sebor wrote:
> On 2/4/20 6:05 PM, Martin Sebor wrote:
>> GCC diagnoses declarations of function aliases whose type doesn't
>> match that of the target (ditto for attribute weakref).  It doesn't
>> yet diagnose such incompatbilities for variable aliases but that's
>> just an oversight that I will try to remember to correct in GCC 11.
>> The attached patch updates the manual to make it clear that
>> aliases must have the same type as their targets, or the behavior
>> is undefined (and may be diagnosed).
> 
> On further review I noticed a few problems with the documentation
> of attribute weakref.  The manual says that
> 
>    Without a target, given as an argument to weakref or to alias,
>    weakref is equivalent to weak.
> 
> and
> 
>    At present, a declaration to which weakref is attached can only
>    be static.
> 
> However, GCC accepts the following declaration:
> 
>    extern int x (void) __attribute__ ((weakref));
> 
> so the second sentence isn't correct without qualification (unlike
> weakref, a weak declaration must be external).
> 
> Another documentation problem is with the example in the manual that
> says that this declaration:
> 
>    static int x() __attribute__ ((weakref ("y")));
> 
>    /* is equivalent to... */
> 
>    static int x() __attribute__ ((weak, weakref, alias ("y")));
> 
> but GCC rejects the latter with
> 
>    error: weak declaration of ‘x’ must be public
> 
> Changing the latter from static to extern changes the error to
> 
>    error: ‘weakref’ symbol ‘x’ must have static linkage
> 
> So clearly two declarations with the two sets of attributes are not
> equivalent, either extern or static.
> 
> I've fixed up these documentation problems in the attached revision
> of the original patch.  I also mention that besides their types
> having to match, the alias must have the same size and alignment
> as the target.
> 
> Martin
> 
> PS I also noted that when the function is then also used GCC issues:
> 
>    warning: ‘weakref’ attribute should be accompanied with an ‘alias’ 
> attribute
> 
> This matches what the manual says in "Without arguments, it should
> be accompanied by an alias attribute naming the target symbol."
> 
> But when the weakref function is not used there is no warning.
> That seems like an unfortunate side-effect of the choice of issuing
> the warning a little too late (waning from the front-end it would
> make it consistent regardless of whether the function was used, and
> it would highlight the omission even in translation units that
> define the weakref without using it).

On 2/4/20 6:05 PM, Martin Sebor wrote:
> GCC diagnoses declarations of function aliases whose type doesn't
> match that of the target (ditto for attribute weakref).  It doesn't
> yet diagnose such incompatbilities for variable aliases but that's
> just an oversight that I will try to remember to correct in GCC 11.
> The attached patch updates the manual to make it clear that
> aliases must have the same type as their targets, or the behavior
> is undefined (and may be diagnosed).
> 

This is OK with the "The the" typo in this hunk fixed:

> @@ -3932,7 +3935,8 @@ and linker.
>  The @code{weakref} attribute marks a declaration as a weak reference.
>  Without arguments, it should be accompanied by an @code{alias} attribute
>  naming the target symbol.  Optionally, the @var{target} may be given as
> -an argument to @code{weakref} itself.  In either case, @code{weakref}
> +an argument to @code{weakref} itself.  The the @var{target} must have
> +the same type as the declaration.  In either case, @code{weakref}
>  implicitly marks the declaration as @code{weak}.  Without a
>  @var{target}, given as an argument to @code{weakref} or to @code{alias},
>  @code{weakref} is equivalent to @code{weak}.

-Sandra

On 2/5/20 1:13 PM, Martin Sebor wrote:

> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index ec99c38a607..3634ce1c423 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -2557,8 +2557,11 @@ __attribute__ ((access (write_only, 1, 2), access (read_write, 3))) int fgets (c
>  
>  @item alias ("@var{target}")
>  @cindex @code{alias} function attribute
> -The @code{alias} attribute causes the declaration to be emitted as an
> -alias for another symbol, which must be specified.  For instance,
> +The @code{alias} attribute causes the declaration to be emitted as an alias
> +for another symbol, which must have been previously declared with the same
> +type, and for variables same size and alignment.  Declaring an alias with
> +a different type than the target is undefined and may be diagnosed.  For
> +instance, an alias for another symbol, which must be specified.  For instance,
>  
>  @smallexample
>  void __f () @{ /* @r{Do something.} */; @}

This is kind of garbled.  Do you have an extraneous sentence beginning 
"For instance" in there?  It doesn't flow from the text you added before 
that.

> @@ -3919,31 +3922,41 @@ results in warning on line 5.
>  
>  @item weak
>  @cindex @code{weak} function attribute
> -The @code{weak} attribute causes the declaration to be emitted as a weak
> -symbol rather than a global.  This is primarily useful in defining
> -library functions that can be overridden in user code, though it can
> -also be used with non-function declarations.  Weak symbols are supported
> -for ELF targets, and also for a.out targets when using the GNU assembler
> -and linker.
> +The @code{weak} attribute causes a declaration of an external symbol
> +to be emitted as a weak symbol rather than a global.  This is primarily
> +useful in defining library functions that can be overridden in user code,
> +though it can also be used with non-function declarations.  The overriding
> +symbol must have the same type, and for variables size, and alignment as
> +the weak symbol.  Weak symbols are supported for ELF targets, and also for
> +a.out targets when using the GNU assembler and linker.

The "for variables" clause is awkward and has a comma in the wrong 
place.  I'd split this into a separate sentence, like

The overriding symbol must have the same type as the weak symbol.  If it 
is a variable it must also have the same size and alignment.

>  @item weakref
>  @itemx weakref ("@var{target}")
>  @cindex @code{weakref} function attribute
>  The @code{weakref} attribute marks a declaration as a weak reference.
>  Without arguments, it should be accompanied by an @code{alias} attribute
> -naming the target symbol.  Optionally, the @var{target} may be given as
> -an argument to @code{weakref} itself.  In either case, @code{weakref}
> -implicitly marks the declaration as @code{weak}.  Without a
> -@var{target}, given as an argument to @code{weakref} or to @code{alias},
> -@code{weakref} is equivalent to @code{weak}.
> +naming the target symbol.  Alernatively, @var{target} may be given as

s/Alernatively/Alternatively/

> +an argument to @code{weakref} itself, naming the target definition of
> +the alias.  The the @var{target} must have the same type, and for

s/The the/The/

> +variables also the same size and alignment as the declaration.  In either

Same comments above re splitting this into two sentences to avoid 
awkward wording.

The rest of this patch looks OK.

-Sandra

On 2/13/20 3:55 PM, Sandra Loosemore wrote:
> On 2/5/20 1:13 PM, Martin Sebor wrote:
> 
>> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
>> index ec99c38a607..3634ce1c423 100644
>> --- a/gcc/doc/extend.texi
>> +++ b/gcc/doc/extend.texi
>> @@ -2557,8 +2557,11 @@ __attribute__ ((access (write_only, 1, 2), 
>> access (read_write, 3))) int fgets (c
>>
>>  @item alias ("@var{target}")
>>  @cindex @code{alias} function attribute
>> -The @code{alias} attribute causes the declaration to be emitted as an
>> -alias for another symbol, which must be specified.  For instance,
>> +The @code{alias} attribute causes the declaration to be emitted as an 
>> alias
>> +for another symbol, which must have been previously declared with the 
>> same
>> +type, and for variables same size and alignment.  Declaring an alias 
>> with
>> +a different type than the target is undefined and may be diagnosed.  For
>> +instance, an alias for another symbol, which must be specified.  For 
>> instance,
>>
>>  @smallexample
>>  void __f () @{ /* @r{Do something.} */; @}
> 
> This is kind of garbled.  Do you have an extraneous sentence beginning 
> "For instance" in there?  It doesn't flow from the text you added before 
> that.

That does look garbled.  I've fixed that in the revised patch.

> 
>> @@ -3919,31 +3922,41 @@ results in warning on line 5.
>>
>>  @item weak
>>  @cindex @code{weak} function attribute
>> -The @code{weak} attribute causes the declaration to be emitted as a weak
>> -symbol rather than a global.  This is primarily useful in defining
>> -library functions that can be overridden in user code, though it can
>> -also be used with non-function declarations.  Weak symbols are supported
>> -for ELF targets, and also for a.out targets when using the GNU assembler
>> -and linker.
>> +The @code{weak} attribute causes a declaration of an external symbol
>> +to be emitted as a weak symbol rather than a global.  This is primarily
>> +useful in defining library functions that can be overridden in user 
>> code,
>> +though it can also be used with non-function declarations.  The 
>> overriding
>> +symbol must have the same type, and for variables size, and alignment as
>> +the weak symbol.  Weak symbols are supported for ELF targets, and 
>> also for
>> +a.out targets when using the GNU assembler and linker.
> 
> The "for variables" clause is awkward and has a comma in the wrong 
> place.  I'd split this into a separate sentence, like
> 
> The overriding symbol must have the same type as the weak symbol.  If it 
> is a variable it must also have the same size and alignment.

Agreed, that looks better.

> 
>>  @item weakref
>>  @itemx weakref ("@var{target}")
>>  @cindex @code{weakref} function attribute
>>  The @code{weakref} attribute marks a declaration as a weak reference.
>>  Without arguments, it should be accompanied by an @code{alias} attribute
>> -naming the target symbol.  Optionally, the @var{target} may be given as
>> -an argument to @code{weakref} itself.  In either case, @code{weakref}
>> -implicitly marks the declaration as @code{weak}.  Without a
>> -@var{target}, given as an argument to @code{weakref} or to @code{alias},
>> -@code{weakref} is equivalent to @code{weak}.
>> +naming the target symbol.  Alernatively, @var{target} may be given as
> 
> s/Alernatively/Alternatively/
> 
>> +an argument to @code{weakref} itself, naming the target definition of
>> +the alias.  The the @var{target} must have the same type, and for
> 
> s/The the/The/
> 
>> +variables also the same size and alignment as the declaration.  In 
>> either
> 
> Same comments above re splitting this into two sentences to avoid 
> awkward wording.

I've spit it up into multiple sentences as you suggested.  Attached
is the updated revision that I plan to commit unless you (or anyone
else) have further suggestions.

Thanks for the review!

Martin

> 
> The rest of this patch looks OK.
> 
> -Sandra

On 2/14/20 11:30 AM, Martin Sebor wrote:
>
> [snip]
> Attached
> is the updated revision that I plan to commit unless you (or anyone
> else) have further suggestions.

This version is OK to commit.

-Sandra