Re: [V1][PATCH 1/3] Provide element_count attribute to flexible array member field (PR108896)

[Qing Zhao <qing.zhao@oracle.com>]

Hi, Joseph,

I studied c_parser_attribute_arguments and related source code, 
and also studied PLACEHOLDER_EXPR and related source code. 

Right now, I still cannot decide what’s the best user-interface should be for the 
argument of the new attribute “element_count". (Or the new attribute 
name “counted_by” as suggested by Kees). 

There are 3 possible interfaces for the argument of the new attribute “counted_by”:

The first 2 interfaces are the following A and B:

A. the argument of the new attribute “counted_by” is a string as the current patch:

struct trailing_array_A {
  Int count;
  int array_A[] __attribute ((counted_by (“count"))); 
};

B. The argument of the new attribute “counted_by” is an identifier that can be
 accepted by “c_parser_attribute_arguments”:

struct trailing_array_B {
  Int count;
  int array_B[] __attribute ((counted_by (count))); 
};

To implement this interface,  we need to adjust “attribute_takes_identifier_p” to 
accept the new attribute “counted_by” and then interpret this new identifier  “count” as a 
field of the containing structure by looking at the name.  (Otherwise, the identifier “count”
will be treated as an identifier in the current scope, which is not declared yet)

Comparing B with A, I don’t see too much benefit, either from user-interface point of view, 
or from implementation point of view.  

For implementation, both A and B need to search the fields of the containing structure by 
the name of the field “count”.

For user interface, I think that A and B are similar.

In addition to consider the user-interface and implementation, another concern is the possibility
 to extend the argument of this new attribute to an expression in the future, for example:

struct trailing_array_F {
  Int count;
  int array_F[] __attribute ((counted_by (count * 4))); 
};

In the above struct “trailing_array_F”, the argument of the attribute “counted_by” is “count * 4”, which
is an expression.  

If we plan to extend the argument of this new attribute to an expression, then neither A nor B is
good, right?

For this purpose, it might be cleaner to introduce a new syntax similar as the designator as mentioned in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108896 as following, i.e, the approach C:

C. The argument of the new attribute “counted_by” is a new designator identifier that will be parsed as
The field of the containing structure:

struct trailing_array_C {
  Int count;
  int array_C[] __attribute ((counted_by (.count))); 
};

I think that after the C FE accepts this new designator syntax as the argument of the attribute, then it’s
very easy to extend the argument to an arbitrary expression later. 

For the implementation of this approach, my current thinking is: 

1. Update the routine “c_parser_postfix_expression” (is this the right place? ) to accept the new designator syntax.
2. Use “PLACEHOLDER_EXPR” to represent the containing structure, and build a COMPONENT_REF to hold
    the argument of the attribute in the IR.
3. When using this attribute in middle-end or sanitizer, use SUBSTITUTE_PLACEHOLDER_IN_EXPR(EXP, OBJ)
    to get the size info in IR. 

So, right now, I think that we might need to take the approach C?

What’s your opinion and suggestions?

Thanks a lot for your help.

Qing


> On Jun 7, 2023, at 6:05 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> 
> On Wed, 7 Jun 2023, Qing Zhao via Gcc-patches wrote:
> 
>> Are you suggesting to use identifier directly as the argument of the attribute?
>> I tried this in the beginning, however, the current parser for the attribute argument can not identify that this identifier is a field identifier inside the same structure. 
>> 
>> For example:
>> 
>> int count;
>> struct trailing_array_7 {
>>  Int count;
>>  int array_7[] __attribute ((element_count (count))); 
>> };
>> 
>> The identifier “count” inside the attribute will refer to the variable 
>> “int count” outside of the structure.
> 
> c_parser_attribute_arguments is supposed to allow an identifier as an 
> attribute argument - and not look it up (the user of the attribute would 
> later need to look it up in the context of the containing structure).  
> Callers use attribute_takes_identifier_p to determine which attributes 
> take identifiers (versus expressions) as arguments, which would need 
> updating to cover the new attribute.
> 
> There is a ??? comment about the case where the identifier is declared as 
> a type name.  That would simply be one of the cases carried over from the 
> old Bison parser, and it would seem reasonable to remove that 
> special-casing so that the attribute works even when the identifier is 
> declared as a typedef name as an ordinary identifier, since it's fine for 
> structure members to have the same name as a typedef name.
> 
> Certainly taking an identifier directly seems like cleaner syntax than 
> taking a string that then needs reinterpreting as an identifier.
> 
> -- 
> Joseph S. Myers
> joseph@codesourcery.com