[testsuite] Allow builtin-has-attribute-* to run as far as possible on targets without alias support.

Hi Martin,

A)
Some of the builtin-has-attribute tests fail because a sub-set of them need symbol alias support.
Darwin has only support for weak aliases and therefore we need to skip these.

However, the sub-set is small, and I am reluctant to throw out the entire set for the sake of a small number, so I propose to wrap that small number in #ifndef that can be enabled by targets without the necessary support (Darwin is not the only one, just the most frequently tested and therefore often “guilty” of finding the problem ;) )

It’s a tricky trade-off between having too many test-cases and having test cases that try to cover too many circumstances...

B) [builtin-has-attribute-4.c]
I am concerned by the diagnostics for the lack of support for the “protected” mode (Darwin doesn’t have this, at least at present).

B.1 the reported diagnostic appears on the closing brace of the function, rather than on the statement that triggers it (line 233).
B.2 I think you’re perhaps missing a %< %> pair - there’s no ‘’ around “protected".
B.3. there are a bunch of other lines with the “protected” visibility marking, but no diagnostic (perhaps that’s intended, I am not sure).

Addressing B is a separate issue from making the current tests pass, it might not be appropriate at this stage .. it’s more of a “head’s up”.

as for the test fixes, OK for trunk?
Iain
    
gcc/testsuite/
    
    	* c-c++-common/builtin-has-attribute-3.c: Skip tests requiring symbol
	alias support.
    	* c-c++-common/builtin-has-attribute-4.c: Likewise.
	Append match for warning that ‘protected’ attribute is not supported.

On 1/7/19 9:55 AM, Iain Sandoe wrote:
> Hi Martin,
> 
> A)
> Some of the builtin-has-attribute tests fail because a sub-set of them need symbol alias support.
> Darwin has only support for weak aliases and therefore we need to skip these.
> 
> However, the sub-set is small, and I am reluctant to throw out the entire set for the sake of a small number, so I propose to wrap that small number in #ifndef that can be enabled by targets without the necessary support (Darwin is not the only one, just the most frequently tested and therefore often “guilty” of finding the problem ;) )
> 
> It’s a tricky trade-off between having too many test-cases and having test cases that try to cover too many circumstances...
> 
> B) [builtin-has-attribute-4.c]
> I am concerned by the diagnostics for the lack of support for the “protected” mode (Darwin doesn’t have this, at least at present).
> 
> B.1 the reported diagnostic appears on the closing brace of the function, rather than on the statement that triggers it (line 233).
> B.2 I think you’re perhaps missing a %< %> pair - there’s no ‘’ around “protected".
> B.3. there are a bunch of other lines with the “protected” visibility marking, but no diagnostic (perhaps that’s intended, I am not sure).
> 
> Addressing B is a separate issue from making the current tests pass, it might not be appropriate at this stage .. it’s more of a “head’s up”.
> 
> as for the test fixes, OK for trunk?
> Iain
>     
> gcc/testsuite/
>     
>     	* c-c++-common/builtin-has-attribute-3.c: Skip tests requiring symbol
> 	alias support.
>     	* c-c++-common/builtin-has-attribute-4.c: Likewise.
> 	Append match for warning that ‘protected’ attribute is not supported.
> 
> diff --git a/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c b/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c
OK
jeff

On 1/7/19 9:55 AM, Iain Sandoe wrote:
> Hi Martin,
> 
> A)
> Some of the builtin-has-attribute tests fail because a sub-set of them need symbol alias support.
> Darwin has only support for weak aliases and therefore we need to skip these.
> 
> However, the sub-set is small, and I am reluctant to throw out the entire set for the sake of a small number, so I propose to wrap that small number in #ifndef that can be enabled by targets without the necessary support (Darwin is not the only one, just the most frequently tested and therefore often “guilty” of finding the problem ;) )
> 
> It’s a tricky trade-off between having too many test-cases and having test cases that try to cover too many circumstances...
> 
> B) [builtin-has-attribute-4.c]
> I am concerned by the diagnostics for the lack of support for the “protected” mode (Darwin doesn’t have this, at least at present).
> 
> B.1 the reported diagnostic appears on the closing brace of the function, rather than on the statement that triggers it (line 233).
> B.2 I think you’re perhaps missing a %< %> pair - there’s no ‘’ around “protected".
> B.3. there are a bunch of other lines with the “protected” visibility marking, but no diagnostic (perhaps that’s intended, I am not sure).
> 
> Addressing B is a separate issue from making the current tests pass, it might not be appropriate at this stage .. it’s more of a “head’s up”.
> 
> as for the test fixes, OK for trunk?

Jeff already approved the patch so let me just say thanks for
the cleanup and the heads up.

Using an #ifdef like your did makes sense to me for now.  If other
targets need something similar it might be worth considering whether
to add an effective-target for them instead of enumerating them in
the test, and also change the test to verify that the appropriate
warning is issued.  I'll try to remember to look into it.

I suspect the missing quotes from around "protected" in the warning
comed from gcc/config/darwin.c:

     warning (OPT_Wattributes, "protected visibility attribute "
              "not supported in this configuration; ignored");

Let me add them along with a test for them.  I'll see if I can
quickly tell also why the warning isn't issued consistently.

Martin

> Iain
>      
> gcc/testsuite/
>      
>      	* c-c++-common/builtin-has-attribute-3.c: Skip tests requiring symbol
> 	alias support.
>      	* c-c++-common/builtin-has-attribute-4.c: Likewise.
> 	Append match for warning that ‘protected’ attribute is not supported.
> 
> diff --git a/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c b/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c
> index f048059..5b2e5c7 100644
> --- a/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c
> +++ b/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c
> @@ -1,7 +1,9 @@
>   /* Verify __builtin_has_attribute return value for functions.
>      { dg-do compile }
>      { dg-options "-Wall -ftrack-macro-expansion=0" }
> -   { dg-options "-Wall -Wno-narrowing -Wno-unused-local-typedefs -ftrack-macro-expansion=0" { target c++ } }  */
> +   { dg-options "-Wall -Wno-narrowing -Wno-unused-local-typedefs -ftrack-macro-expansion=0" { target c++ } }
> +   { dg-additional-options "-DSKIP_ALIAS" { target *-*-darwin* } }
> +*/
>   
>   #define ATTR(...) __attribute__ ((__VA_ARGS__))
>   
> @@ -27,7 +29,9 @@ extern "C"
>   #endif
>   ATTR (noreturn) void fnoreturn (void) { __builtin_abort (); }
>   
> +#ifndef SKIP_ALIAS
>   ATTR (alias ("fnoreturn")) void falias (void);
> +#endif
>   
>   #define A(expect, sym, attr)						\
>     typedef int Assert [1 - 2 * !(__builtin_has_attribute (sym, attr) == expect)]
> @@ -114,7 +118,7 @@ void test_alloc_size_malloc (void)
>     A (1, fmalloc_size_3, malloc);
>   }
>   
> -
> +#ifndef SKIP_ALIAS
>   void test_alias (void)
>   {
>     A (0, fnoreturn, alias);
> @@ -123,7 +127,7 @@ void test_alias (void)
>     A (0, falias, alias ("falias"));
>     A (0, falias, alias ("fnone"));
>   }
> -
> +#endif
>   
>   void test_cold_hot (void)
>   {
> diff --git a/gcc/testsuite/c-c++-common/builtin-has-attribute-4.c b/gcc/testsuite/c-c++-common/builtin-has-attribute-4.c
> index d56ef6b..0c36cfc 100644
> --- a/gcc/testsuite/c-c++-common/builtin-has-attribute-4.c
> +++ b/gcc/testsuite/c-c++-common/builtin-has-attribute-4.c
> @@ -1,7 +1,9 @@
>   /* Verify __builtin_has_attribute return value for variables.
>      { dg-do compile }
>      { dg-options "-Wall -ftrack-macro-expansion=0" }
> -   { dg-options "-Wall -Wno-narrowing -Wno-unused -ftrack-macro-expansion=0" { target c++ } }  */
> +   { dg-options "-Wall -Wno-narrowing -Wno-unused -ftrack-macro-expansion=0" { target c++ } }
> +   { dg-additional-options "-DSKIP_ALIAS" { target *-*-darwin* } }
> +*/
>   
>   #define ATTR(...) __attribute__ ((__VA_ARGS__))
>   
> @@ -45,6 +47,7 @@ void test_aligned (void)
>   }
>   
>   
> +#ifndef SKIP_ALIAS
>   int vtarget;
>   extern ATTR (alias ("vtarget")) int valias;
>   
> @@ -55,7 +58,7 @@ void test_alias (void)
>     A (1, valias, alias ("vtarget"));
>     A (0, valias, alias ("vnone"));
>   }
> -
> +#endif
>   
>   void test_cleanup (void)
>   {
> @@ -227,7 +230,7 @@ void test_vector_size (void)
>   ATTR (visibility ("default")) int vdefault;
>   ATTR (visibility ("hidden")) int vhidden;
>   ATTR (visibility ("internal")) int vinternal;
> -ATTR (visibility ("protected")) int vprotected;
> +ATTR (visibility ("protected")) int vprotected;
>   
>   void test_visibility (void)
>   {
> @@ -280,6 +283,6 @@ void test_weak (void)
>   
>     A (1, var_init_weak, weak);
>     A (1, var_uninit_weak, weak);
> -}
> +} /* { dg-warning "protected visibility attribute not supported" "" { target { *-*-darwin* } } } */
>   
>   /* { dg-prune-output "specifies less restrictive attribute" } */
>

Hi Martin,

> On 8 Jan 2019, at 00:25, Martin Sebor <msebor@gmail.com> wrote:
> 
> On 1/7/19 9:55 AM, Iain Sandoe wrote:
>> Hi Martin,
>> A)
>> Some of the builtin-has-attribute tests fail because a sub-set of them need symbol alias support.
>> Darwin has only support for weak aliases and therefore we need to skip these.
>> However, the sub-set is small, and I am reluctant to throw out the entire set for the sake of a small number, so I propose to wrap that small number in #ifndef that can be enabled by targets without the necessary support (Darwin is not the only one, just the most frequently tested and therefore often “guilty” of finding the problem ;) )
>> It’s a tricky trade-off between having too many test-cases and having test cases that try to cover too many circumstances...
>> B) [builtin-has-attribute-4.c]
>> I am concerned by the diagnostics for the lack of support for the “protected” mode (Darwin doesn’t have this, at least at present).
>> B.1 the reported diagnostic appears on the closing brace of the function, rather than on the statement that triggers it (line 233).
>> B.2 I think you’re perhaps missing a %< %> pair - there’s no ‘’ around “protected".
>> B.3. there are a bunch of other lines with the “protected” visibility marking, but no diagnostic (perhaps that’s intended, I am not sure).
>> Addressing B is a separate issue from making the current tests pass, it might not be appropriate at this stage .. it’s more of a “head’s up”.
>> as for the test fixes, OK for trunk?
> 
> Jeff already approved the patch so let me just say thanks for
> the cleanup and the heads up.

I will apply the patch as is (probably my tomorrow AM) for now - and then we can adjust if there’s any change to the diagnostics.
> 
> Using an #ifdef like your did makes sense to me for now.  If other
> targets need something similar it might be worth considering whether
> to add an effective-target for them instead of enumerating them in
> the test, and also change the test to verify that the appropriate
> warning is issued.  I'll try to remember to look into it.
> 
> I suspect the missing quotes from around "protected" in the warning
> comed from gcc/config/darwin.c:
> 
>    warning (OPT_Wattributes, "protected visibility attribute "
>             "not supported in this configuration; ignored”);

Ah..  and that’s a “warning” rather than a “warning_at” which explains the placement on the final brace.  Without looking at the context, not sure what location info we might have there.  Note to self: better audit other stuff in Darwin’s port-specific attrs.

> Let me add them along with a test for them.  

There is a test in builtin-has-attribute-4.c, by default and ….
>> +} /* { dg-warning "protected visibility attribute not supported" "" { target { *-*-darwin* } } } */

> I'll see if I can
> quickly tell also why the warning isn't issued consistently.

… this might produce a bunch more warnings.

thanks
Iain