Message ID | 0f3f1395-adac-8b5f-82e4-e656bf1207fb@gmail.com |
---|---|
State | New |
Headers | show |
Series | accept all C integer types in function parameters referenced by alloc_align (PR 88363) | expand |
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
PR c/88363 - alloc_align attribute doesn't accept enumerated arguments gcc/c-family/ChangeLog: PR c/88363 * c-attribs.c (positional_argument): Also accept enumerated types. gcc/testsuite/ChangeLog: PR c/88363 * c-c++-common/attributes-4.c: New test. gcc/ChangeLog: PR c/88363 * doc/extend.texi (attribute alloc_align, alloc_size): Update. Index: gcc/c-family/c-attribs.c =================================================================== --- gcc/c-family/c-attribs.c (revision 266963) +++ gcc/c-family/c-attribs.c (working copy) @@ -630,11 +630,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 +641,10 @@ 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 bool, enums, and other types that + match INTEGRAL_TYPE_P. */ + type_match = INTEGRAL_TYPE_P (argtype); else type_match = TREE_CODE (argtype) == code; Index: gcc/doc/extend.texi =================================================================== --- gcc/doc/extend.texi (revision 266963) +++ gcc/doc/extend.texi (working copy) @@ -2471,7 +2471,8 @@ 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. 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 +2496,8 @@ 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. 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,46 @@ +/* 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 }; + +/* Using bool here is of marginal utility but it's accepted nonetheless. */ +ATTR (alloc_align (1)) void* falloc_align_bool (bool); +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_size (1)) void* falloc_size_bool (bool); +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; + +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" } */