accept all C integer types in function parameters referenced by alloc_align (PR 88363)

Some of my testing exposed a minor problem in GCC 9's validation
of the type of function parameters referred to by attribute
positional arguments.  Whereas GCC 8 accepts all C integer types,
including enumerated types, such as:

   enum AllocAlign { Align16 = 16, Align32 = 32 };

   __attribute__ ((alloc_align (1))) void*
   alloc (size_t, enum AllocAlign)

GCC 9 only accepts signed and unsigned integer types.  This change
(introduced by myself) was unintentional, and a fix for it is in
the attached trivial patch.  I plan to commit it without approval
in the next day or so unless any concerns or suggestions come up.

Tested on x86_64-linux.

Martin

On Mon, Dec 10, 2018 at 04:30:11PM -0700, Martin Sebor wrote:
> Some of my testing exposed a minor problem in GCC 9's validation
> of the type of function parameters referred to by attribute
> positional arguments.  Whereas GCC 8 accepts all C integer types,
> including enumerated types, such as:
> 
>   enum AllocAlign { Align16 = 16, Align32 = 32 };
> 
>   __attribute__ ((alloc_align (1))) void*
>   alloc (size_t, enum AllocAlign)
> 
> GCC 9 only accepts signed and unsigned integer types.  This change
> (introduced by myself) was unintentional, and a fix for it is in
> the attached trivial patch.  I plan to commit it without approval
> in the next day or so unless any concerns or suggestions come up.

There is nothing obvious about this, so please don't commit it without
approval.  GCC 8 and older used to accept
even float or void * or struct arguments and just ignored them.
I think we need to discuss what types we want to allow without warnings and
what we should warn.
As I wrote in the PR, I believe e.g. using alloc_align/alloc_size with
bool/_Bool is just a clear bug, you can store 0 or 1 in there, but e.g.
alignment 0 doesn't make sense.
Enums are on the border line, I'll defer to C/C++ maintainers whether we
want to include that or not.

	Jakub

On 12/11/18 2:17 AM, Jakub Jelinek wrote:
> On Mon, Dec 10, 2018 at 04:30:11PM -0700, Martin Sebor wrote:
>> Some of my testing exposed a minor problem in GCC 9's validation
>> of the type of function parameters referred to by attribute
>> positional arguments.  Whereas GCC 8 accepts all C integer types,
>> including enumerated types, such as:
>>
>>    enum AllocAlign { Align16 = 16, Align32 = 32 };
>>
>>    __attribute__ ((alloc_align (1))) void*
>>    alloc (size_t, enum AllocAlign)
>>
>> GCC 9 only accepts signed and unsigned integer types.  This change
>> (introduced by myself) was unintentional, and a fix for it is in
>> the attached trivial patch.  I plan to commit it without approval
>> in the next day or so unless any concerns or suggestions come up.
> 
> There is nothing obvious about this, so please don't commit it without
> approval.  GCC 8 and older used to accept
> even float or void * or struct arguments and just ignored them.
> I think we need to discuss what types we want to allow without warnings and
> what we should warn.
> As I wrote in the PR, I believe e.g. using alloc_align/alloc_size with
> bool/_Bool is just a clear bug, you can store 0 or 1 in there, but e.g.
> alignment 0 doesn't make sense.
> Enums are on the border line, I'll defer to C/C++ maintainers whether we
> want to include that or not.

I'd think we should allow (unscoped) enums in most places where we want 
an integer constant.

Jason

On Tue, Dec 11, 2018 at 08:17:26AM +0100, Jakub Jelinek wrote:
> On Mon, Dec 10, 2018 at 04:30:11PM -0700, Martin Sebor wrote:
> > Some of my testing exposed a minor problem in GCC 9's validation
> > of the type of function parameters referred to by attribute
> > positional arguments.  Whereas GCC 8 accepts all C integer types,
> > including enumerated types, such as:
> > 
> >   enum AllocAlign { Align16 = 16, Align32 = 32 };
> > 
> >   __attribute__ ((alloc_align (1))) void*
> >   alloc (size_t, enum AllocAlign)
> > 
> > GCC 9 only accepts signed and unsigned integer types.  This change
> > (introduced by myself) was unintentional, and a fix for it is in
> > the attached trivial patch.  I plan to commit it without approval
> > in the next day or so unless any concerns or suggestions come up.
> 
> There is nothing obvious about this, so please don't commit it without
> approval.  GCC 8 and older used to accept

I agree that this isn't an obvious change.

> even float or void * or struct arguments and just ignored them.
> I think we need to discuss what types we want to allow without warnings and
> what we should warn.
> As I wrote in the PR, I believe e.g. using alloc_align/alloc_size with
> bool/_Bool is just a clear bug, you can store 0 or 1 in there, but e.g.
> alignment 0 doesn't make sense.
> Enums are on the border line, I'll defer to C/C++ maintainers whether we
> want to include that or not.

For C, I'd allow them (and I think I've made a change to that effect in the
past in the C FE).

Marek

On 12/11/18 12:17 AM, Jakub Jelinek wrote:
> On Mon, Dec 10, 2018 at 04:30:11PM -0700, Martin Sebor wrote:
>> Some of my testing exposed a minor problem in GCC 9's validation
>> of the type of function parameters referred to by attribute
>> positional arguments.  Whereas GCC 8 accepts all C integer types,
>> including enumerated types, such as:
>>
>>    enum AllocAlign { Align16 = 16, Align32 = 32 };
>>
>>    __attribute__ ((alloc_align (1))) void*
>>    alloc (size_t, enum AllocAlign)
>>
>> GCC 9 only accepts signed and unsigned integer types.  This change
>> (introduced by myself) was unintentional, and a fix for it is in
>> the attached trivial patch.  I plan to commit it without approval
>> in the next day or so unless any concerns or suggestions come up.
> 
> There is nothing obvious about this, so please don't commit it without
> approval.

See (*) below.

> GCC 8 and older used to accept
> even float or void * or struct arguments and just ignored them.
> I think we need to discuss what types we want to allow without warnings and
> what we should warn.

Silently accepting invalid arguments like void* or structs and
proceeding to ignore them is in my view a bug.  Clang diagnoses
both and that seems like the appropriate choice, so that's what
I implemented in r266195 back in November.  This patch doesn't
change that; rather, it accepts more of the things that were
accepted previously and that r266195 unintentionally rejected:
bool, enums, and wide characters.  I welcome feedback on this
decision which is why I posted the patch here before checking
it in.

> As I wrote in the PR, I believe e.g. using alloc_align/alloc_size with
> bool/_Bool is just a clear bug, you can store 0 or 1 in there, but e.g.
> alignment 0 doesn't make sense.

I'm fine with being more restrictive and rejecting bool.  Clang
accepts it but I agree that it's far more likely a bug in user
code (and an oversight in Clang).

> Enums are on the border line, I'll defer to C/C++ maintainers whether we
> want to include that or not.

 From Jason's and Marek's responses it sounds like accepting enums
here is appropriate.  (Clang also accepts them.)  I will go ahead
and make the change for bool alone then.

> 
> 	Jakub
> 

[*] The change in the patch is obvious enough to me.  All it
does is accept more of the things that are accepted by GCC 8
(enums, bools, wchar_t, etc.) and that inadvertently started
to be rejected as a result of my prior change.   That the rules
can be made more restrictive is something different.

I recently brought up the question of the write w/o approval
policy on the gcc list:

   https://gcc.gnu.org/ml/gcc/2018-11/msg00165.html

looking for clarification.  Except for Jeff's comment (which
I'm afraid didn't really clarify things), didn't get any.

You (the maintainers) have put it in place.  If you don't intend
for the rest of us to make use of it, or if it's not meant to be
interpreted to give us the freedom to decide what is or isn't
obvious, then change it. But it's disingenuous to claim that "We
don't want to get overly restrictive about checkin policies" and
then chastise people each time they say they might check something
in on their own.

On Tue, Dec 11, 2018 at 09:59:27AM -0700, Martin Sebor wrote:
> [*] The change in the patch is obvious enough to me.  All it
> does is accept more of the things that are accepted by GCC 8
> (enums, bools, wchar_t, etc.) and that inadvertently started
> to be rejected as a result of my prior change.   That the rules
> can be made more restrictive is something different.
 
Obvious changes should be obvious to anyone, not just the committer, IMHO.  I
don't think we should make the rules more restrictive; what we have in place
seems to have worked fine and I would have thought it's clear that changing
what the compiler accepts will never be an obvious change, unlike, say, fixing
a test that fails with -m32 because it uses 'unsigned long' instead of size_t.

> I recently brought up the question of the write w/o approval
> policy on the gcc list:
> 
>   https://gcc.gnu.org/ml/gcc/2018-11/msg00165.html
> 
> looking for clarification.  Except for Jeff's comment (which
> I'm afraid didn't really clarify things), didn't get any.
> 
> You (the maintainers) have put it in place.  If you don't intend
> for the rest of us to make use of it, or if it's not meant to be
> interpreted to give us the freedom to decide what is or isn't
> obvious, then change it. But it's disingenuous to claim that "We
> don't want to get overly restrictive about checkin policies" and
> then chastise people each time they say they might check something
> in on their own.

...or fixing typos in comments and formatting fixes should be obvious, adding
new tests for fixed bugs likely as well, but outlining semantics in a comment
doesn't strike me as obvious at all.  "When in doubt, ask for a review."

Marek

On Tue, 11 Dec 2018, Martin Sebor wrote:

> I recently brought up the question of the write w/o approval
> policy on the gcc list:
> 
>   https://gcc.gnu.org/ml/gcc/2018-11/msg00165.html
> 
> looking for clarification.  Except for Jeff's comment (which
> I'm afraid didn't really clarify things), didn't get any.

I think "will the person who objects to my work the most be able to find a 
fault with my fix?" in the policy on obviousness is clear enough.  A 
policy decision on what is or is not part of a language extension can't be 
obvious, and nor can determining subtle questions of exactly what the 
definition of some internal interface is or should be.

On 12/11/18 11:15 AM, Marek Polacek wrote:
> On Tue, Dec 11, 2018 at 09:59:27AM -0700, Martin Sebor wrote:
>> [*] The change in the patch is obvious enough to me.  All it
>> does is accept more of the things that are accepted by GCC 8
>> (enums, bools, wchar_t, etc.) and that inadvertently started
>> to be rejected as a result of my prior change.   That the rules
>> can be made more restrictive is something different.
>   
> Obvious changes should be obvious to anyone, not just the committer, IMHO.  I
> don't think we should make the rules more restrictive; 

I was referring to the rules of what expression the compiler accepts
as positional parameters.

> what we have in place
> seems to have worked fine and I would have thought it's clear that changing
> what the compiler accepts will never be an obvious change, unlike, say, fixing
> a test that fails with -m32 because it uses 'unsigned long' instead of size_t.

If that's the intent it should be stated in the policy then.  Seems
simple enough:

   Obvious fixes <ins>that have no impact on constructs accepted
   or rejected by the compiler</ins> can be committed without
   prior approval.

I have no problem with this clarification.  I just wouldn't have
thought it to apply to restoring previous behavior that I myself
had inadvertently removed (i.e., fixing my own regression).

>> I recently brought up the question of the write w/o approval
>> policy on the gcc list:
>>
>>    https://gcc.gnu.org/ml/gcc/2018-11/msg00165.html
>>
>> looking for clarification.  Except for Jeff's comment (which
>> I'm afraid didn't really clarify things), didn't get any.
>>
>> You (the maintainers) have put it in place.  If you don't intend
>> for the rest of us to make use of it, or if it's not meant to be
>> interpreted to give us the freedom to decide what is or isn't
>> obvious, then change it. But it's disingenuous to claim that "We
>> don't want to get overly restrictive about checkin policies" and
>> then chastise people each time they say they might check something
>> in on their own.
> 
> ...or fixing typos in comments and formatting fixes should be obvious, adding
> new tests for fixed bugs likely as well, but outlining semantics in a comment
> doesn't strike me as obvious at all.  "When in doubt, ask for a review."

I was not in doubt, but I posted the patch for review just
the same, didn't I?

If by "outlining semantics in a comment" you are referring to
the patch mentioned in the other thread then if such changes are
also not meant to fall under obvious then again, update the policy:

   Obvious fixes <ins>that have no impact on constructs accepted
   or rejected by the compiler and that do not outline the semantics
   of GCC internals in comments</ins> can be committed without prior
   approval.

In both of these cases I posted the patch and invited feedback
to make sure that was seemed clearly correct to me wouldn't be
viewed differently by others.  That still seems perfectly
appropriate to me.  But if even that isn't acceptable then update
the policy to make it clear that posting a patch and saying that
you'll commit it if no one objects is also against the rules.

Martin

On 12/11/18 11:15 AM, Joseph Myers wrote:
> On Tue, 11 Dec 2018, Martin Sebor wrote:
> 
>> I recently brought up the question of the write w/o approval
>> policy on the gcc list:
>>
>>    https://gcc.gnu.org/ml/gcc/2018-11/msg00165.html
>>
>> looking for clarification.  Except for Jeff's comment (which
>> I'm afraid didn't really clarify things), didn't get any.
> 
> I think "will the person who objects to my work the most be able to find a
> fault with my fix?" in the policy on obviousness is clear enough.  A
> policy decision on what is or is not part of a language extension can't be
> obvious, and nor can determining subtle questions of exactly what the
> definition of some internal interface is or should be.

Anything that someone might find fault with is so broad as to
remove the ability to make the judgment in any case.  Reviewers
have been known to find fault with the slightest things, from
trivial formatting nits, to punctuation in comments, to names
of variables, to the location of new tests, to ChangeLogs.

If the policy's intent is not to let people make judgment calls
then it ought to be updated.  I have no proposal for changes to
it at the moment, but I don't see how anyone can reasonably
object to someone posting a patch and saying "if there are no
objections I will go ahead and commit this change because I think
it's obviously correct."  If even that is against the policy then
change it to make that clear (though I sincerely hope that isn't
so).

Martin

On Tue, Dec 11, 2018 at 2:46 PM Martin Sebor <msebor@gmail.com> wrote:
> On 12/11/18 11:15 AM, Joseph Myers wrote:
> > On Tue, 11 Dec 2018, Martin Sebor wrote:
> >
> >> I recently brought up the question of the write w/o approval
> >> policy on the gcc list:
> >>
> >>    https://gcc.gnu.org/ml/gcc/2018-11/msg00165.html
> >>
> >> looking for clarification.  Except for Jeff's comment (which
> >> I'm afraid didn't really clarify things), didn't get any.
> >
> > I think "will the person who objects to my work the most be able to find a
> > fault with my fix?" in the policy on obviousness is clear enough.  A
> > policy decision on what is or is not part of a language extension can't be
> > obvious, and nor can determining subtle questions of exactly what the
> > definition of some internal interface is or should be.
>
> Anything that someone might find fault with is so broad as to
> remove the ability to make the judgment in any case.  Reviewers
> have been known to find fault with the slightest things, from
> trivial formatting nits, to punctuation in comments, to names
> of variables, to the location of new tests, to ChangeLogs.
>
> If the policy's intent is not to let people make judgment calls
> then it ought to be updated.  I have no proposal for changes to
> it at the moment, but I don't see how anyone can reasonably
> object to someone posting a patch and saying "if there are no
> objections I will go ahead and commit this change because I think
> it's obviously correct."  If even that is against the policy then
> change it to make that clear (though I sincerely hope that isn't
> so).

I don't think anyone objected to your mail, they were just disagreeing
that the patch was obvious.  That is also a judgment call.  IMO
there's no need to have an ironclad policy here, since the
consequences of a particular change being "wrongly" consider either
obvious or non-obvious are small.

Jason

On 12/11/18 12:17 AM, Jakub Jelinek wrote:
> On Mon, Dec 10, 2018 at 04:30:11PM -0700, Martin Sebor wrote:
>> Some of my testing exposed a minor problem in GCC 9's validation
>> of the type of function parameters referred to by attribute
>> positional arguments.  Whereas GCC 8 accepts all C integer types,
>> including enumerated types, such as:
>>
>>    enum AllocAlign { Align16 = 16, Align32 = 32 };
>>
>>    __attribute__ ((alloc_align (1))) void*
>>    alloc (size_t, enum AllocAlign)
>>
>> GCC 9 only accepts signed and unsigned integer types.  This change
>> (introduced by myself) was unintentional, and a fix for it is in
>> the attached trivial patch.  I plan to commit it without approval
>> in the next day or so unless any concerns or suggestions come up.

Attached is an updated version of the patch that restores
the original behavior for the positional argument validation
(i.e., prior to r266195) for integral types except bool as
discussed.

Martin
Index: gcc/c-family/c-attribs.c
===================================================================
--- gcc/c-family/c-attribs.c	(revision 266966)
+++ gcc/c-family/c-attribs.c	(working copy)
@@ -498,10 +498,11 @@ attribute_takes_identifier_p (const_tree attr_id)
 
 /* Verify that argument value POS at position ARGNO to attribute NAME
    applied to function TYPE refers to a function parameter at position
-   POS and the expected type CODE.  If so, return POS after default
-   conversions, if any.  Otherwise, issue appropriate warnings and
-   return null.  A non-zero 1-based ARGNO should be passed ib by
-   callers only for attributes with more than one argument.  */
+   POS and the expected type CODE.  Treat CODE == INTEGER_TYPE as
+   matching all C integral types except bool.  If successful, return
+   POS after default conversions, if any.  Otherwise, issue appropriate
+   warnings and return null.  A non-zero 1-based ARGNO should be passed
+   in by callers only for attributes with more than one argument.  */
 
 tree
 positional_argument (const_tree fntype, const_tree atname, tree pos,
@@ -630,11 +631,11 @@ positional_argument (const_tree fntype, const_tree
 	  return NULL_TREE;
 	}
 
-      /* Where the expected code is STRING_CST accept any pointer
-	 to a narrow character type, qualified or otherwise.  */
       bool type_match;
       if (code == STRING_CST && POINTER_TYPE_P (argtype))
 	{
+	  /* Where the expected code is STRING_CST accept any pointer
+	     to a narrow character type, qualified or otherwise.  */
 	  tree type = TREE_TYPE (argtype);
 	  type = TYPE_MAIN_VARIANT (type);
 	  type_match = (type == char_type_node
@@ -641,6 +642,11 @@ positional_argument (const_tree fntype, const_tree
 			|| type == signed_char_type_node
 			|| type == unsigned_char_type_node);
 	}
+      else if (code == INTEGER_TYPE)
+	/* For integers, accept enums, wide characters and other types
+	   that match INTEGRAL_TYPE_P except for bool.  */
+	type_match = (INTEGRAL_TYPE_P (argtype)
+		      && TREE_CODE (argtype) != BOOLEAN_TYPE);
       else
 	type_match = TREE_CODE (argtype) == code;
 
Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 266966)
+++ gcc/doc/extend.texi	(working copy)
@@ -2471,7 +2471,9 @@ The @code{aligned} attribute can also be used for
 @item alloc_align (@var{position})
 @cindex @code{alloc_align} function attribute
 The @code{alloc_align} attribute may be applied to a function that
-returns a pointer and takes at least one argument of an integer type.
+returns a pointer and takes at least one argument of an integer or
+enumerated type, but not @code{bool}.  Arguments of other types are
+diagnosed.
 It indicates that the returned pointer is aligned on a boundary given
 by the function argument at @var{position}.  Meaningful alignments are
 powers of 2 greater than one.  GCC uses this information to improve
@@ -2495,7 +2497,9 @@ given by parameter 1.
 @itemx alloc_size (@var{position-1}, @var{position-2})
 @cindex @code{alloc_size} function attribute
 The @code{alloc_size} attribute may be applied to a function that
-returns a pointer and takes at least one argument of an integer type.
+returns a pointer and takes at least one argument of an integer or
+enumerated type, but not @code{bool}.  Arguments of other types are
+diagnosed.
 It indicates that the returned pointer points to memory whose size is
 given by the function argument at @var{position-1}, or by the product
 of the arguments at @var{position-1} and @var{position-2}.  Meaningful
Index: gcc/testsuite/c-c++-common/attributes-4.c
===================================================================
--- gcc/testsuite/c-c++-common/attributes-4.c	(nonexistent)
+++ gcc/testsuite/c-c++-common/attributes-4.c	(working copy)
@@ -0,0 +1,47 @@
+/* PR c/88363 - alloc_align attribute doesn't accept enumerated arguments
+   Verify that attribute positional arguments can refer to all C integer
+   types in both C and C++.
+   { dg-do compile }
+   { dg-options "-Wall" }
+   { dg-options "-Wall -Wno-c++-compat" { target c } } */
+
+#define ATTR(...) __attribute__ ((__VA_ARGS__))
+
+#if __cplusplus == 199711L
+typedef __CHAR16_TYPE__ char16_t;
+typedef __CHAR32_TYPE__ char32_t;
+#elif !__cplusplus
+typedef _Bool           bool;
+typedef __CHAR16_TYPE__ char16_t;
+typedef __CHAR32_TYPE__ char32_t;
+typedef __WCHAR_TYPE__  wchar_t;
+#endif
+
+enum A { A0 };
+
+ATTR (alloc_align (1)) void* falloc_align_char (char);
+ATTR (alloc_align (1)) void* falloc_align_char16 (char16_t);
+ATTR (alloc_align (1)) void* falloc_align_char32 (char32_t);
+ATTR (alloc_align (1)) void* falloc_align_wchar (wchar_t);
+/* Using an enum might make sense in an API that limits the alignments
+   it accepts to just the set of the defined enumerators.   */
+ATTR (alloc_align (1)) void* falloc_align_enum (enum A);
+ATTR (alloc_align (1)) void* falloc_align_int128 (__int128_t);
+
+
+ATTR (alloc_align (1)) void* falloc_size_char (char);
+ATTR (alloc_size (1)) void* falloc_size_char16 (char16_t);
+ATTR (alloc_size (1)) void* falloc_size_char32 (char32_t);
+ATTR (alloc_size (1)) void* falloc_size_wchar (wchar_t);
+ATTR (alloc_size (1)) void* falloc_size_enum (enum A);
+ATTR (alloc_align (1)) void* falloc_size_int128 (__int128_t);
+
+
+typedef struct { int i; } S;
+
+/* Using bool is most likely a bug and so diagnosed even though
+   it could be accepted.  None of the other types makes sense.  */
+ATTR (alloc_align (1)) void* falloc_align_bool (bool);      /* { dg-warning "attribute argument value .1. refers to parameter type .\(_Bool|bool\)" } */
+ATTR (alloc_align (1)) void* falloc_align_float (float);    /* { dg-warning "attribute argument value .1. refers to parameter type .float" } */
+ATTR (alloc_align (1)) void* falloc_align_voidp (void*);    /* { dg-warning "attribute argument value .1. refers to parameter type .void ?\\\*" } */
+ATTR (alloc_align (1)) void* falloc_align_struct (S);       /* { dg-warning "attribute argument value .1. refers to parameter type .S" } */

On Tue, Dec 11, 2018 at 01:36:58PM -0700, Martin Sebor wrote:
> Attached is an updated version of the patch that restores
> the original behavior for the positional argument validation
> (i.e., prior to r266195) for integral types except bool as
> discussed.

I thought Jason wanted to also warn for scoped enums in C++.

> +      else if (code == INTEGER_TYPE)
> +	/* For integers, accept enums, wide characters and other types
> +	   that match INTEGRAL_TYPE_P except for bool.  */
> +	type_match = (INTEGRAL_TYPE_P (argtype)
> +		      && TREE_CODE (argtype) != BOOLEAN_TYPE);

So it would better be:
	type_match = (TREE_CODE (argtype) == INTEGER_TYPE
		      || (TREE_CODE (argtype) == ENUMERAL_TYPE
			  && !ENUM_IS_SCOPED (argtype)));

> --- gcc/doc/extend.texi	(revision 266966)
> +++ gcc/doc/extend.texi	(working copy)
> @@ -2471,7 +2471,9 @@ The @code{aligned} attribute can also be used for
>  @item alloc_align (@var{position})
>  @cindex @code{alloc_align} function attribute
>  The @code{alloc_align} attribute may be applied to a function that
> -returns a pointer and takes at least one argument of an integer type.
> +returns a pointer and takes at least one argument of an integer or
> +enumerated type, but not @code{bool}.  Arguments of other types are
> +diagnosed.

The keyword is _Bool in C, so wouldn't it be better to say but not Boolean
type or but not @code{bool} or @code{_Bool}?
And it should mention the scoped enumeration types for C++ too
(+ testsuite coverage for them).

	Jakub

On 12/11/18 1:47 PM, Jakub Jelinek wrote:
> On Tue, Dec 11, 2018 at 01:36:58PM -0700, Martin Sebor wrote:
>> Attached is an updated version of the patch that restores
>> the original behavior for the positional argument validation
>> (i.e., prior to r266195) for integral types except bool as
>> discussed.
> 
> I thought Jason wanted to also warn for scoped enums in C++.

I missed that.  It seems needlessly restrictive to me to reject
the preferred kind of an enum when ordinary enums are accepted.
Jason, can you confirm that you really want a warning for B
below when there is none for A (GCC 8 doesn't complain about
either, Clang complains about both, ICC about neither when
using alloc_size -- it doesn't understand alloc_align):

   enum A { /* ... */ };
   __attribute__ ((alloc_align (1))) void* f (A);

   enum class B { /* ... */ };
   __attribute__ ((alloc_align (1))) void* g (B);

The only use case I can think of for enums is in APIs that try
to restrict the available choices of alignment to those of
the enumerators.  In that use case, I would expect it to make
no difference whether the enum is ordinary or the scoped kind.

> 
>> +      else if (code == INTEGER_TYPE)
>> +	/* For integers, accept enums, wide characters and other types
>> +	   that match INTEGRAL_TYPE_P except for bool.  */
>> +	type_match = (INTEGRAL_TYPE_P (argtype)
>> +		      && TREE_CODE (argtype) != BOOLEAN_TYPE);
> 
> So it would better be:
> 	type_match = (TREE_CODE (argtype) == INTEGER_TYPE
> 		      || (TREE_CODE (argtype) == ENUMERAL_TYPE
> 			  && !ENUM_IS_SCOPED (argtype)));
> 
>> --- gcc/doc/extend.texi	(revision 266966)
>> +++ gcc/doc/extend.texi	(working copy)
>> @@ -2471,7 +2471,9 @@ The @code{aligned} attribute can also be used for
>>   @item alloc_align (@var{position})
>>   @cindex @code{alloc_align} function attribute
>>   The @code{alloc_align} attribute may be applied to a function that
>> -returns a pointer and takes at least one argument of an integer type.
>> +returns a pointer and takes at least one argument of an integer or
>> +enumerated type, but not @code{bool}.  Arguments of other types are
>> +diagnosed.
> 
> The keyword is _Bool in C, so wouldn't it be better to say but not Boolean
> type or but not @code{bool} or @code{_Bool}?

It makes little difference.  Established practice in the manual
is @code{bool} (14 occurrences).  There are just three instances
of @code{_Bool} in prose in doc/*.texi.  It's simpler to refer
to @code{bool} unless we are specifically talking about the C
keyword/type.

Martin

On Tue, Dec 11, 2018 at 03:46:37PM -0700, Martin Sebor wrote:
> On 12/11/18 1:47 PM, Jakub Jelinek wrote:
> > On Tue, Dec 11, 2018 at 01:36:58PM -0700, Martin Sebor wrote:
> > > Attached is an updated version of the patch that restores
> > > the original behavior for the positional argument validation
> > > (i.e., prior to r266195) for integral types except bool as
> > > discussed.
> > 
> > I thought Jason wanted to also warn for scoped enums in C++.
> 
> I missed that.  It seems needlessly restrictive to me to reject
> the preferred kind of an enum when ordinary enums are accepted.
> Jason, can you confirm that you really want a warning for B
> below when there is none for A (GCC 8 doesn't complain about
> either, Clang complains about both, ICC about neither when
> using alloc_size -- it doesn't understand alloc_align):
> 
>   enum A { /* ... */ };
>   __attribute__ ((alloc_align (1))) void* f (A);
> 
>   enum class B { /* ... */ };
>   __attribute__ ((alloc_align (1))) void* g (B);
> 
> The only use case I can think of for enums is in APIs that try
> to restrict the available choices of alignment to those of
> the enumerators.  In that use case, I would expect it to make
> no difference whether the enum is ordinary or the scoped kind.

The reason was that C++ scoped enumerations don't implicitly convert to
integral types.

Marek

On 12/11/18 3:52 PM, Marek Polacek wrote:
> On Tue, Dec 11, 2018 at 03:46:37PM -0700, Martin Sebor wrote:
>> On 12/11/18 1:47 PM, Jakub Jelinek wrote:
>>> On Tue, Dec 11, 2018 at 01:36:58PM -0700, Martin Sebor wrote:
>>>> Attached is an updated version of the patch that restores
>>>> the original behavior for the positional argument validation
>>>> (i.e., prior to r266195) for integral types except bool as
>>>> discussed.
>>>
>>> I thought Jason wanted to also warn for scoped enums in C++.
>>
>> I missed that.  It seems needlessly restrictive to me to reject
>> the preferred kind of an enum when ordinary enums are accepted.
>> Jason, can you confirm that you really want a warning for B
>> below when there is none for A (GCC 8 doesn't complain about
>> either, Clang complains about both, ICC about neither when
>> using alloc_size -- it doesn't understand alloc_align):
>>
>>    enum A { /* ... */ };
>>    __attribute__ ((alloc_align (1))) void* f (A);
>>
>>    enum class B { /* ... */ };
>>    __attribute__ ((alloc_align (1))) void* g (B);
>>
>> The only use case I can think of for enums is in APIs that try
>> to restrict the available choices of alignment to those of
>> the enumerators.  In that use case, I would expect it to make
>> no difference whether the enum is ordinary or the scoped kind.
> 
> The reason was that C++ scoped enumerations don't implicitly convert to
> integral types.

I'm not sure we're talking about the same thing.  There is no
conversion in the use case I described, the attribute argument
just refers to the function parameter, and the function is called
with an argument of the enumerated type of the parameter.  Like
this:

   enum class Alignment { a4 = 4, a8 = 8 };

   __attribute__ ((alloc_align (1))) void*
   aligned_alloc (Alignment, size_t);

   void *p = aligned_alloc (Alignment::a8, 32);

My question is: if we think it makes sense to accept this use
case with ordinary enums why would we not want to make it possible
with scoped enums?  People tend to think of the latter as preferable
over the former.

Martin

On 12/11/18 6:08 PM, Martin Sebor wrote:
> On 12/11/18 3:52 PM, Marek Polacek wrote:
>> On Tue, Dec 11, 2018 at 03:46:37PM -0700, Martin Sebor wrote:
>>> On 12/11/18 1:47 PM, Jakub Jelinek wrote:
>>>> On Tue, Dec 11, 2018 at 01:36:58PM -0700, Martin Sebor wrote:
>>>>> Attached is an updated version of the patch that restores
>>>>> the original behavior for the positional argument validation
>>>>> (i.e., prior to r266195) for integral types except bool as
>>>>> discussed.
>>>>
>>>> I thought Jason wanted to also warn for scoped enums in C++.
>>>
>>> I missed that.  It seems needlessly restrictive to me to reject
>>> the preferred kind of an enum when ordinary enums are accepted.
>>> Jason, can you confirm that you really want a warning for B
>>> below when there is none for A (GCC 8 doesn't complain about
>>> either, Clang complains about both, ICC about neither when
>>> using alloc_size -- it doesn't understand alloc_align):
>>>
>>>    enum A { /* ... */ };
>>>    __attribute__ ((alloc_align (1))) void* f (A);
>>>
>>>    enum class B { /* ... */ };
>>>    __attribute__ ((alloc_align (1))) void* g (B);
>>>
>>> The only use case I can think of for enums is in APIs that try
>>> to restrict the available choices of alignment to those of
>>> the enumerators.  In that use case, I would expect it to make
>>> no difference whether the enum is ordinary or the scoped kind.
>>
>> The reason was that C++ scoped enumerations don't implicitly convert to
>> integral types.
> 
> I'm not sure we're talking about the same thing.  There is no
> conversion in the use case I described, the attribute argument
> just refers to the function parameter, and the function is called
> with an argument of the enumerated type of the parameter.  Like
> this:
> 
>    enum class Alignment { a4 = 4, a8 = 8 };
> 
>    __attribute__ ((alloc_align (1))) void*
>    aligned_alloc (Alignment, size_t);
> 
>    void *p = aligned_alloc (Alignment::a8, 32);
> 
> My question is: if we think it makes sense to accept this use
> case with ordinary enums why would we not want to make it possible
> with scoped enums?  People tend to think of the latter as preferable
> over the former.

OK, I suppose it's reasonable to allow scoped enums as well.

Jason

On 12/11/18 4:19 PM, Jason Merrill wrote:
> On 12/11/18 6:08 PM, Martin Sebor wrote:
>> On 12/11/18 3:52 PM, Marek Polacek wrote:
>>> On Tue, Dec 11, 2018 at 03:46:37PM -0700, Martin Sebor wrote:
>>>> On 12/11/18 1:47 PM, Jakub Jelinek wrote:
>>>>> On Tue, Dec 11, 2018 at 01:36:58PM -0700, Martin Sebor wrote:
>>>>>> Attached is an updated version of the patch that restores
>>>>>> the original behavior for the positional argument validation
>>>>>> (i.e., prior to r266195) for integral types except bool as
>>>>>> discussed.
>>>>>
>>>>> I thought Jason wanted to also warn for scoped enums in C++.
>>>>
>>>> I missed that.  It seems needlessly restrictive to me to reject
>>>> the preferred kind of an enum when ordinary enums are accepted.
>>>> Jason, can you confirm that you really want a warning for B
>>>> below when there is none for A (GCC 8 doesn't complain about
>>>> either, Clang complains about both, ICC about neither when
>>>> using alloc_size -- it doesn't understand alloc_align):
>>>>
>>>>    enum A { /* ... */ };
>>>>    __attribute__ ((alloc_align (1))) void* f (A);
>>>>
>>>>    enum class B { /* ... */ };
>>>>    __attribute__ ((alloc_align (1))) void* g (B);
>>>>
>>>> The only use case I can think of for enums is in APIs that try
>>>> to restrict the available choices of alignment to those of
>>>> the enumerators.  In that use case, I would expect it to make
>>>> no difference whether the enum is ordinary or the scoped kind.
>>>
>>> The reason was that C++ scoped enumerations don't implicitly convert to
>>> integral types.
>>
>> I'm not sure we're talking about the same thing.  There is no
>> conversion in the use case I described, the attribute argument
>> just refers to the function parameter, and the function is called
>> with an argument of the enumerated type of the parameter.  Like
>> this:
>>
>>    enum class Alignment { a4 = 4, a8 = 8 };
>>
>>    __attribute__ ((alloc_align (1))) void*
>>    aligned_alloc (Alignment, size_t);
>>
>>    void *p = aligned_alloc (Alignment::a8, 32);
>>
>> My question is: if we think it makes sense to accept this use
>> case with ordinary enums why would we not want to make it possible
>> with scoped enums?  People tend to think of the latter as preferable
>> over the former.
> 
> OK, I suppose it's reasonable to allow scoped enums as well.

Are there any other suggestions for changes or should I take
this as an approval to commit the updated patch?

https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00740.html

Martin

Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00740.html

On 12/18/18 2:42 PM, Martin Sebor wrote:
> On 12/11/18 4:19 PM, Jason Merrill wrote:
>> On 12/11/18 6:08 PM, Martin Sebor wrote:
>>> On 12/11/18 3:52 PM, Marek Polacek wrote:
>>>> On Tue, Dec 11, 2018 at 03:46:37PM -0700, Martin Sebor wrote:
>>>>> On 12/11/18 1:47 PM, Jakub Jelinek wrote:
>>>>>> On Tue, Dec 11, 2018 at 01:36:58PM -0700, Martin Sebor wrote:
>>>>>>> Attached is an updated version of the patch that restores
>>>>>>> the original behavior for the positional argument validation
>>>>>>> (i.e., prior to r266195) for integral types except bool as
>>>>>>> discussed.
>>>>>>
>>>>>> I thought Jason wanted to also warn for scoped enums in C++.
>>>>>
>>>>> I missed that.  It seems needlessly restrictive to me to reject
>>>>> the preferred kind of an enum when ordinary enums are accepted.
>>>>> Jason, can you confirm that you really want a warning for B
>>>>> below when there is none for A (GCC 8 doesn't complain about
>>>>> either, Clang complains about both, ICC about neither when
>>>>> using alloc_size -- it doesn't understand alloc_align):
>>>>>
>>>>>    enum A { /* ... */ };
>>>>>    __attribute__ ((alloc_align (1))) void* f (A);
>>>>>
>>>>>    enum class B { /* ... */ };
>>>>>    __attribute__ ((alloc_align (1))) void* g (B);
>>>>>
>>>>> The only use case I can think of for enums is in APIs that try
>>>>> to restrict the available choices of alignment to those of
>>>>> the enumerators.  In that use case, I would expect it to make
>>>>> no difference whether the enum is ordinary or the scoped kind.
>>>>
>>>> The reason was that C++ scoped enumerations don't implicitly convert to
>>>> integral types.
>>>
>>> I'm not sure we're talking about the same thing.  There is no
>>> conversion in the use case I described, the attribute argument
>>> just refers to the function parameter, and the function is called
>>> with an argument of the enumerated type of the parameter.  Like
>>> this:
>>>
>>>    enum class Alignment { a4 = 4, a8 = 8 };
>>>
>>>    __attribute__ ((alloc_align (1))) void*
>>>    aligned_alloc (Alignment, size_t);
>>>
>>>    void *p = aligned_alloc (Alignment::a8, 32);
>>>
>>> My question is: if we think it makes sense to accept this use
>>> case with ordinary enums why would we not want to make it possible
>>> with scoped enums?  People tend to think of the latter as preferable
>>> over the former.
>>
>> OK, I suppose it's reasonable to allow scoped enums as well.
> 
> Are there any other suggestions for changes or should I take
> this as an approval to commit the updated patch?
> 
> https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00740.html
> 
> Martin

This patch is OK.

On Tue, Dec 11, 2018 at 01:36:58PM -0700, Martin Sebor wrote:
> --- gcc/testsuite/c-c++-common/attributes-4.c	(nonexistent)
> +++ gcc/testsuite/c-c++-common/attributes-4.c	(working copy)
> @@ -0,0 +1,47 @@
...
> +ATTR (alloc_align (1)) void* falloc_align_int128 (__int128_t);
...
> +ATTR (alloc_align (1)) void* falloc_size_int128 (__int128_t);

The test FAILs on 32-bit targets which don't have __int128_t type.
As there is no associated diagnostics expected with these, I've just guarded
those with #ifdef __SIZEOF_INT128__, tested on x86_64-linux -m32/-m64
and committed as obvious to trunk:

2019-01-06  Jakub Jelinek  <jakub@redhat.com>

	PR c/88363
	* c-c++-common/attributes-4.c (falloc_align_int128,
	falloc_size_int128): Guard with #ifdef __SIZEOF_INT128__.

--- gcc/testsuite/c-c++-common/attributes-4.c.jj	2019-01-05 12:06:13.416101609 +0100
+++ gcc/testsuite/c-c++-common/attributes-4.c	2019-01-06 11:23:13.198901344 +0100
@@ -26,7 +26,9 @@ ATTR (alloc_align (1)) void* falloc_alig
 /* Using an enum might make sense in an API that limits the alignments
    it accepts to just the set of the defined enumerators.   */
 ATTR (alloc_align (1)) void* falloc_align_enum (enum A);
+#ifdef __SIZEOF_INT128__
 ATTR (alloc_align (1)) void* falloc_align_int128 (__int128_t);
+#endif
 
 
 ATTR (alloc_align (1)) void* falloc_size_char (char);
@@ -34,7 +36,9 @@ ATTR (alloc_size (1)) void* falloc_size_
 ATTR (alloc_size (1)) void* falloc_size_char32 (char32_t);
 ATTR (alloc_size (1)) void* falloc_size_wchar (wchar_t);
 ATTR (alloc_size (1)) void* falloc_size_enum (enum A);
+#ifdef __SIZEOF_INT128__
 ATTR (alloc_align (1)) void* falloc_size_int128 (__int128_t);
+#endif
 
 
 typedef struct { int i; } S;


	Jakub