Re: [PATCH] accept all C integer types in function parameters referenced by alloc_align (PR 88363)

[Martin Sebor <msebor@gmail.com>]

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.