Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BoundsSafety] WIP: Make 'counted_by' work for pointer fields; late parsing for 'counted_by' on decl attr position #87596

Closed

Conversation

rapidsna
Copy link
Contributor

@rapidsna rapidsna commented Apr 4, 2024

This contains a minimum change to make 'counted_by' available for pointer fields and make late parsing work for 'counted_by' on declaration attribute position.

Here are TODOs:

  • As we promised in the RFC discussion, separately guard the late parsing logic under a feature flag (e.g., -fexperimental-late-parse-attributes)
  • Make it an error to use 'counted_by' on size-less types (e.g., forward declared structs)

TODOs that may be done as separate PRs:

  • Fix some test cases that don't work properly for anonymous structs
  • Make late parsing work for the type attribute positions
  • Have __builtin_dynamic_object_size and UBSan's array-bounds checks take advantage of counted_by on pointer fields (I believe @bwendling has been working on it).
  • With -fbounds-safety, make 'counted_by' and the late parsing logic available for more subjects (parameters, variables, return types, etc.)

test-edit

Copy link

github-actions bot commented Apr 4, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.


struct at_pointer {
int count;
struct size_unknown *__counted_by(count) buf; // expected-error{{'counted_by' cannot be applied to an sized type}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: emit an error for 'counted_by' on unknown size like this and update the error message here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fine, because count is declared before use—unless I'm getting type attributes confused with field attributes.. Also, there's a grammar-o in the message: to _an_ sized type.

@@ -2497,7 +2501,9 @@ class Parser : public CodeCompletionHandler {

void ParseStructDeclaration(
ParsingDeclSpec &DS,
llvm::function_ref<void(ParsingFieldDeclarator &)> FieldsCallback);
llvm::function_ref<void(ParsingFieldDeclarator &, Decl *&)>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding using Decl*& as a parameter? Would it make more sense as a return value? All the uses I can see unconditionally write to the variable so this isn't an inout parameter. It seems like its just an out parameter which means it could be a return value instead.


ParsedAttributes Attrs(AttrFactory);

assert(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a diagnostic rather than an assert?


// If the Decl is on a function, add function parameters to the scope.
{
std::unique_ptr<ParseScope> Scope;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we missing tests for this? This looks like its to support __counted_by() on function parameters but I don't think we have that in tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I guess this isn't testable because of the let Subjects = SubjectList<[Field], ErrorDiag>;?

MaybeParseGNUAttributes(attrs);
MaybeParseGNUAttributes(attrs, &LateFieldAttrs);

assert(!getLangOpts().CPlusPlus);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove the assert? Are we restricting late parsing to C only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@delcypher C++ already has late parsing support for C++ classes and structs and I believe it's handled separately in ParseDeclCXX.cpp. I added the assertion to ensure that the code path isn't taken in C++. We should run clang tests if that breaks anything.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to combine C++'s late parsing with your implementation? Or is there little overlap?

@@ -1599,6 +1599,13 @@ defm double_square_bracket_attributes : BoolFOption<"double-square-bracket-attri
LangOpts<"DoubleSquareBracketAttributes">, DefaultTrue, PosFlag<SetTrue>,
NegFlag<SetFalse>>;

defm experimental_late_parse_attributes : BoolFOption<"experimental-late-parse-attributes",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rapidsna FYI. Here's a sketch of an experimental late parsing flag. It's not quite done yet and I think it's big enough that it deserves it's own PR. Just pushing it here to give you an idea of my plans

@delcypher delcypher force-pushed the count-on-pointers-and-late-parsing branch from ef0838d to 0b9c5a8 Compare April 12, 2024 01:54
MaybeParseGNUAttributes(attrs);
MaybeParseGNUAttributes(attrs, &LateFieldAttrs);

assert(!getLangOpts().CPlusPlus);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to combine C++'s late parsing with your implementation? Or is there little overlap?


struct at_pointer {
int count;
struct size_unknown *__counted_by(count) buf; // expected-error{{'counted_by' cannot be applied to an sized type}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fine, because count is declared before use—unless I'm getting type attributes confused with field attributes.. Also, there's a grammar-o in the message: to _an_ sized type.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I'm planning on having a separate test/Sema/attr-counted-by-for-pointers.c test file for pointers and rename the current file something like test/Sema/attr-counted-by-for-flexible-array-members.c.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fine, because count is declared before use—unless I'm getting type attributes confused with field attributes..

struct size_unknown *__counted_by(count) buf indicates that buf has sizeof(struct size_unknown) * count bytes. So the problem is here that the compiler doesn't know what's the actual byte size of buf because sizeof(struct unknown_size) is not available here.

@delcypher delcypher force-pushed the count-on-pointers-and-late-parsing branch from 0b9c5a8 to 560b9bb Compare April 12, 2024 23:26
delcypher added a commit to delcypher/apple-llvm-project that referenced this pull request Apr 13, 2024
…e parsing when passing an experimental feature flag

This patch changes the `LateParsed` field of `Attr` in `Attr.td` to be
an instantiation of the new `LateAttrParseKind` class. The instation can be one of the following:

* `LateAttrParsingNever` - Corresponds with the false value of `LateParsed` prior to this patch (the default for an attribute).
* `LateAttrParsingAlways` - Corresponds with the true value of `LateParsed` prior to this patch.
* `LateAttrParsingExperimentalOnly` - A new mode described below.

`LateAttrParsingExperimentalOnly` means that the attribute will be late
parsed if the new the `ExperimentalLateParseAttributes` language option (also introduced in
this patch) is enabled and will **not** be late parsed if the language
option is disabled.

The new `ExperimentalLateParseAttributes` language option is controlled
by a new driver and frontend flag
(`-fexperimental-late-parse-attributes`). A driver test is included to
check that the driver passes the flag to CC1.

In this patch the `LateAttrParsingExperimentalOnly` is not adopted by
any attribute so `-fexperimental-late-pase-attributes` and the
corresponding language option currently have no effect on compilation.
This is why this patch does not include any test cases that test
`LateAttrParsingExperimentalOnly`'s behavior.

The motivation for this patch is to be able to late parse the new
`counted_by` attribute but only do so when a feature flag is passed.
This was discussed during the [bounds-safety RFC](https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/68).

Adoption of `LateAttrParsingExperimentalOnly` for the `counted_by`
attributed will be handled separately in another patch (likely
llvm#87596).
@delcypher
Copy link
Contributor

@rapidsna @bwendling I've put the experimental feature flag for late parsing in a separate PR (#88596). I've not updated this PR to be based on that PR yet but will do soon. I figured the code for experimental feature flag was large enough that it deserved its own PR to make things easier to review.

delcypher added a commit to delcypher/apple-llvm-project that referenced this pull request Apr 15, 2024
…e parsing when passing an experimental feature flag

This patch changes the `LateParsed` field of `Attr` in `Attr.td` to be
an instantiation of the new `LateAttrParseKind` class. The instation can be one of the following:

* `LateAttrParsingNever` - Corresponds with the false value of `LateParsed` prior to this patch (the default for an attribute).
* `LateAttrParsingAlways` - Corresponds with the true value of `LateParsed` prior to this patch.
* `LateAttrParsingExperimentalOnly` - A new mode described below.

`LateAttrParsingExperimentalOnly` means that the attribute will be late
parsed if the new the `ExperimentalLateParseAttributes` language option (also introduced in
this patch) is enabled and will **not** be late parsed if the language
option is disabled.

The new `ExperimentalLateParseAttributes` language option is controlled
by a new driver and frontend flag
(`-fexperimental-late-parse-attributes`). A driver test is included to
check that the driver passes the flag to CC1.

In this patch the `LateAttrParsingExperimentalOnly` is not adopted by
any attribute so `-fexperimental-late-pase-attributes` and the
corresponding language option currently have no effect on compilation.
This is why this patch does not include any test cases that test
`LateAttrParsingExperimentalOnly`'s behavior.

The motivation for this patch is to be able to late parse the new
`counted_by` attribute but only do so when a feature flag is passed.
This was discussed during the [bounds-safety RFC](https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/68).

Adoption of `LateAttrParsingExperimentalOnly` for the `counted_by`
attributed will be handled separately in another patch (likely
llvm#87596).
delcypher added a commit to delcypher/apple-llvm-project that referenced this pull request Apr 15, 2024
…e parsing when passing an experimental feature flag

This patch changes the `LateParsed` field of `Attr` in `Attr.td` to be
an instantiation of the new `LateAttrParseKind` class. The instation can be one of the following:

* `LateAttrParsingNever` - Corresponds with the false value of `LateParsed` prior to this patch (the default for an attribute).
* `LateAttrParsingAlways` - Corresponds with the true value of `LateParsed` prior to this patch.
* `LateAttrParsingExperimentalOnly` - A new mode described below.

`LateAttrParsingExperimentalOnly` means that the attribute will be late
parsed if the new the `ExperimentalLateParseAttributes` language option (also introduced in
this patch) is enabled and will **not** be late parsed if the language
option is disabled.

The new `ExperimentalLateParseAttributes` language option is controlled
by a new driver and frontend flag
(`-fexperimental-late-parse-attributes`). A driver test is included to
check that the driver passes the flag to CC1.

In this patch the `LateAttrParsingExperimentalOnly` is not adopted by
any attribute so `-fexperimental-late-pase-attributes` and the
corresponding language option currently have no effect on compilation.
This is why this patch does not include any test cases that test
`LateAttrParsingExperimentalOnly`'s behavior.

The motivation for this patch is to be able to late parse the new
`counted_by` attribute but only do so when a feature flag is passed.
This was discussed during the [bounds-safety RFC](https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/68).

Adoption of `LateAttrParsingExperimentalOnly` for the `counted_by`
attributed will be handled separately in another patch (likely
llvm#87596).
@delcypher delcypher force-pushed the count-on-pointers-and-late-parsing branch from 560b9bb to 56d5049 Compare April 20, 2024 00:43
delcypher added a commit to rapidsna/llvm-project that referenced this pull request Apr 20, 2024
…nded" late parsing mode

This patch changes the `LateParsed` field of `Attr` in `Attr.td` to be
an instantiation of the new `LateAttrParseKind` class. The instation can be one of the following:

* `LateAttrParsingNever` - Corresponds with the false value of `LateParsed` prior to this patch (the default for an attribute).
* `LateAttrParseStandard` - Corresponds with the true value of `LateParsed` prior to this patch.
* `LateAttrParseExperimentalExt` - A new mode described below.

`LateAttrParseExperimentalExt` is an experimental extension to
`LateAttrParseStandard`. Essentially this allows
`Parser::ParseGNUAttributes(...)` to distinguish between these cases:

1. Only `LateAttrParseExperimentalExt` attributes should be late parsed.
2. Both `LateAttrParseExperimentalExt` and `LateAttrParseStandard`
  attributes should be late parsed.

Callers (and indirect callers) of `Parser::ParseGNUAttributes(...)`
indicate the desired behavior by setting a flag in the
`LateParsedAttrList` object that is passed to the function.

In addition to the above, a new driver and frontend flag
(`-fexperimental-late-parse-attributes`) with a corresponding LangOpt
(`ExperimentalLateParseAttributes`) is added that changes how
`LateAttrParseExperimentalExt` attributes are parsed.

* When the flag is disabled (default), in cases where only
  `LateAttrParsingExperimentalOnly` late parsing is requested, the
  attribute will be parsed immediately (i.e. **NOT** late parsed). This
  allows the attribute to act just like a `LateAttrParseStandard`
  attribute when the flag is disabled.

* When the flag is enabled, in cases where only
  `LateAttrParsingExperimentalOnly` late parsing is requested, the
  attribute will be late parsed.

The motivation behind this change is to allow the new `counted_by`
attribute (part of `-fbounds-safety`) to support late parsing but
**only** when `-fexperimental-late-parse-attributes` is enabled. This
attribute needs to support late parsing to allow it to refer to fields
later in a struct definition (or function parameters declared later).
However, there isn't a precedent for supporting late attribute parsing
in C so this flag allows the new behavior to exist in Clang but not be
on by default. This behavior was requested as part of the
`-fbounds-safety` RFC process
(https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/68).

This patch doesn't introduce any uses of `LateAttrParseExperimentalExt`.
This will be added for the `counted_by` attribute in a future patch
(llvm#87596). A consequence is the
new behavior added in this patch is not yet testable. Hence, the lack of
tests covering the new behavior.

rdar://125400257
delcypher added a commit to delcypher/apple-llvm-project that referenced this pull request Apr 20, 2024
…imental late parsing mode "extension"

This patch changes the `LateParsed` field of `Attr` in `Attr.td` to be
an instantiation of the new `LateAttrParseKind` class. The instation can be one of the following:

* `LateAttrParsingNever` - Corresponds with the false value of `LateParsed` prior to this patch (the default for an attribute).
* `LateAttrParseStandard` - Corresponds with the true value of `LateParsed` prior to this patch.
* `LateAttrParseExperimentalExt` - A new mode described below.

`LateAttrParseExperimentalExt` is an experimental extension to
`LateAttrParseStandard`. Essentially this allows
`Parser::ParseGNUAttributes(...)` to distinguish between these cases:

1. Only `LateAttrParseExperimentalExt` attributes should be late parsed.
2. Both `LateAttrParseExperimentalExt` and `LateAttrParseStandard`
  attributes should be late parsed.

Callers (and indirect callers) of `Parser::ParseGNUAttributes(...)`
indicate the desired behavior by setting a flag in the
`LateParsedAttrList` object that is passed to the function.

In addition to the above, a new driver and frontend flag
(`-fexperimental-late-parse-attributes`) with a corresponding LangOpt
(`ExperimentalLateParseAttributes`) is added that changes how
`LateAttrParseExperimentalExt` attributes are parsed.

* When the flag is disabled (default), in cases where only
  `LateAttrParsingExperimentalOnly` late parsing is requested, the
  attribute will be parsed immediately (i.e. **NOT** late parsed). This
  allows the attribute to act just like a `LateAttrParseStandard`
  attribute when the flag is disabled.

* When the flag is enabled, in cases where only
  `LateAttrParsingExperimentalOnly` late parsing is requested, the
  attribute will be late parsed.

The motivation behind this change is to allow the new `counted_by`
attribute (part of `-fbounds-safety`) to support late parsing but
**only** when `-fexperimental-late-parse-attributes` is enabled. This
attribute needs to support late parsing to allow it to refer to fields
later in a struct definition (or function parameters declared later).
However, there isn't a precedent for supporting late attribute parsing
in C so this flag allows the new behavior to exist in Clang but not be
on by default. This behavior was requested as part of the
`-fbounds-safety` RFC process
(https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/68).

This patch doesn't introduce any uses of `LateAttrParseExperimentalExt`.
This will be added for the `counted_by` attribute in a future patch
(llvm#87596). A consequence is the
new behavior added in this patch is not yet testable. Hence, the lack of
tests covering the new behavior.

rdar://125400257
delcypher added a commit to delcypher/apple-llvm-project that referenced this pull request Apr 20, 2024
…imental late parsing mode "extension"

This patch changes the `LateParsed` field of `Attr` in `Attr.td` to be
an instantiation of the new `LateAttrParseKind` class. The instation can be one of the following:

* `LateAttrParsingNever` - Corresponds with the false value of `LateParsed` prior to this patch (the default for an attribute).
* `LateAttrParseStandard` - Corresponds with the true value of `LateParsed` prior to this patch.
* `LateAttrParseExperimentalExt` - A new mode described below.

`LateAttrParseExperimentalExt` is an experimental extension to
`LateAttrParseStandard`. Essentially this allows
`Parser::ParseGNUAttributes(...)` to distinguish between these cases:

1. Only `LateAttrParseExperimentalExt` attributes should be late parsed.
2. Both `LateAttrParseExperimentalExt` and `LateAttrParseStandard`
  attributes should be late parsed.

Callers (and indirect callers) of `Parser::ParseGNUAttributes(...)`
indicate the desired behavior by setting a flag in the
`LateParsedAttrList` object that is passed to the function.

In addition to the above, a new driver and frontend flag
(`-fexperimental-late-parse-attributes`) with a corresponding LangOpt
(`ExperimentalLateParseAttributes`) is added that changes how
`LateAttrParseExperimentalExt` attributes are parsed.

* When the flag is disabled (default), in cases where only
  `LateAttrParsingExperimentalOnly` late parsing is requested, the
  attribute will be parsed immediately (i.e. **NOT** late parsed). This
  allows the attribute to act just like a `LateAttrParseStandard`
  attribute when the flag is disabled.

* When the flag is enabled, in cases where only
  `LateAttrParsingExperimentalOnly` late parsing is requested, the
  attribute will be late parsed.

The motivation behind this change is to allow the new `counted_by`
attribute (part of `-fbounds-safety`) to support late parsing but
**only** when `-fexperimental-late-parse-attributes` is enabled. This
attribute needs to support late parsing to allow it to refer to fields
later in a struct definition (or function parameters declared later).
However, there isn't a precedent for supporting late attribute parsing
in C so this flag allows the new behavior to exist in Clang but not be
on by default. This behavior was requested as part of the
`-fbounds-safety` RFC process
(https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/68).

This patch doesn't introduce any uses of `LateAttrParseExperimentalExt`.
This will be added for the `counted_by` attribute in a future patch
(llvm#87596). A consequence is the
new behavior added in this patch is not yet testable. Hence, the lack of
tests covering the new behavior.

rdar://125400257
@delcypher
Copy link
Contributor

The implementation of the experimental late attribute changed (#88596) so I've rebased this PR on it and fixed it up to work as intended.

@delcypher delcypher force-pushed the count-on-pointers-and-late-parsing branch from 56d5049 to 68b8106 Compare April 23, 2024 01:42
@delcypher
Copy link
Contributor

@rapidsna I've addressed the "Make it an error to use 'counted_by' on size-less types (e.g., forward declared structs)" TODO.

I still need to test this PR properly and write a proper commit message explaining what this does.

delcypher added a commit to delcypher/apple-llvm-project that referenced this pull request Apr 23, 2024
…imental late parsing mode "extension"

This patch changes the `LateParsed` field of `Attr` in `Attr.td` to be
an instantiation of the new `LateAttrParseKind` class. The instation can be one of the following:

* `LateAttrParsingNever` - Corresponds with the false value of `LateParsed` prior to this patch (the default for an attribute).
* `LateAttrParseStandard` - Corresponds with the true value of `LateParsed` prior to this patch.
* `LateAttrParseExperimentalExt` - A new mode described below.

`LateAttrParseExperimentalExt` is an experimental extension to
`LateAttrParseStandard`. Essentially this allows
`Parser::ParseGNUAttributes(...)` to distinguish between these cases:

1. Only `LateAttrParseExperimentalExt` attributes should be late parsed.
2. Both `LateAttrParseExperimentalExt` and `LateAttrParseStandard`
  attributes should be late parsed.

Callers (and indirect callers) of `Parser::ParseGNUAttributes(...)`
indicate the desired behavior by setting a flag in the
`LateParsedAttrList` object that is passed to the function.

In addition to the above, a new driver and frontend flag
(`-fexperimental-late-parse-attributes`) with a corresponding LangOpt
(`ExperimentalLateParseAttributes`) is added that changes how
`LateAttrParseExperimentalExt` attributes are parsed.

* When the flag is disabled (default), in cases where only
  `LateAttrParsingExperimentalOnly` late parsing is requested, the
  attribute will be parsed immediately (i.e. **NOT** late parsed). This
  allows the attribute to act just like a `LateAttrParseStandard`
  attribute when the flag is disabled.

* When the flag is enabled, in cases where only
  `LateAttrParsingExperimentalOnly` late parsing is requested, the
  attribute will be late parsed.

The motivation behind this change is to allow the new `counted_by`
attribute (part of `-fbounds-safety`) to support late parsing but
**only** when `-fexperimental-late-parse-attributes` is enabled. This
attribute needs to support late parsing to allow it to refer to fields
later in a struct definition (or function parameters declared later).
However, there isn't a precedent for supporting late attribute parsing
in C so this flag allows the new behavior to exist in Clang but not be
on by default. This behavior was requested as part of the
`-fbounds-safety` RFC process
(https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/68).

This patch doesn't introduce any uses of `LateAttrParseExperimentalExt`.
This will be added for the `counted_by` attribute in a future patch
(llvm#87596). A consequence is the
new behavior added in this patch is not yet testable. Hence, the lack of
tests covering the new behavior.

rdar://125400257
@delcypher delcypher force-pushed the count-on-pointers-and-late-parsing branch from 68b8106 to 27cef27 Compare April 30, 2024 00:56
delcypher added a commit to delcypher/apple-llvm-project that referenced this pull request Apr 30, 2024
…imental late parsing mode "extension"

This patch changes the `LateParsed` field of `Attr` in `Attr.td` to be
an instantiation of the new `LateAttrParseKind` class. The instation can be one of the following:

* `LateAttrParsingNever` - Corresponds with the false value of `LateParsed` prior to this patch (the default for an attribute).
* `LateAttrParseStandard` - Corresponds with the true value of `LateParsed` prior to this patch.
* `LateAttrParseExperimentalExt` - A new mode described below.

`LateAttrParseExperimentalExt` is an experimental extension to
`LateAttrParseStandard`. Essentially this allows
`Parser::ParseGNUAttributes(...)` to distinguish between these cases:

1. Only `LateAttrParseExperimentalExt` attributes should be late parsed.
2. Both `LateAttrParseExperimentalExt` and `LateAttrParseStandard`
  attributes should be late parsed.

Callers (and indirect callers) of `Parser::ParseGNUAttributes(...)`
indicate the desired behavior by setting a flag in the
`LateParsedAttrList` object that is passed to the function.

In addition to the above, a new driver and frontend flag
(`-fexperimental-late-parse-attributes`) with a corresponding LangOpt
(`ExperimentalLateParseAttributes`) is added that changes how
`LateAttrParseExperimentalExt` attributes are parsed.

* When the flag is disabled (default), in cases where only
  `LateAttrParsingExperimentalOnly` late parsing is requested, the
  attribute will be parsed immediately (i.e. **NOT** late parsed). This
  allows the attribute to act just like a `LateAttrParseStandard`
  attribute when the flag is disabled.

* When the flag is enabled, in cases where only
  `LateAttrParsingExperimentalOnly` late parsing is requested, the
  attribute will be late parsed.

The motivation behind this change is to allow the new `counted_by`
attribute (part of `-fbounds-safety`) to support late parsing but
**only** when `-fexperimental-late-parse-attributes` is enabled. This
attribute needs to support late parsing to allow it to refer to fields
later in a struct definition (or function parameters declared later).
However, there isn't a precedent for supporting late attribute parsing
in C so this flag allows the new behavior to exist in Clang but not be
on by default. This behavior was requested as part of the
`-fbounds-safety` RFC process
(https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/68).

This patch doesn't introduce any uses of `LateAttrParseExperimentalExt`.
This will be added for the `counted_by` attribute in a future patch
(llvm#87596). A consequence is the
new behavior added in this patch is not yet testable. Hence, the lack of
tests covering the new behavior.

rdar://125400257
delcypher added a commit that referenced this pull request Apr 30, 2024
…imental late parsing mode "extension" (#88596)

This patch changes the `LateParsed` field of `Attr` in `Attr.td` to be
an instantiation of the new `LateAttrParseKind` class. The instation can be one of the following:

* `LateAttrParsingNever` - Corresponds with the false value of `LateParsed` prior to this patch (the default for an attribute).
* `LateAttrParseStandard` - Corresponds with the true value of `LateParsed` prior to this patch.
* `LateAttrParseExperimentalExt` - A new mode described below.

`LateAttrParseExperimentalExt` is an experimental extension to
`LateAttrParseStandard`. Essentially this allows
`Parser::ParseGNUAttributes(...)` to distinguish between these cases:

1. Only `LateAttrParseExperimentalExt` attributes should be late parsed.
2. Both `LateAttrParseExperimentalExt` and `LateAttrParseStandard`
  attributes should be late parsed.

Callers (and indirect callers) of `Parser::ParseGNUAttributes(...)`
indicate the desired behavior by setting a flag in the
`LateParsedAttrList` object that is passed to the function.

In addition to the above, a new driver and frontend flag
(`-fexperimental-late-parse-attributes`) with a corresponding LangOpt
(`ExperimentalLateParseAttributes`) is added that changes how
`LateAttrParseExperimentalExt` attributes are parsed.

* When the flag is disabled (default), in cases where only
  `LateAttrParsingExperimentalOnly` late parsing is requested, the
  attribute will be parsed immediately (i.e. **NOT** late parsed). This
  allows the attribute to act just like a `LateAttrParseStandard`
  attribute when the flag is disabled.

* When the flag is enabled, in cases where only
  `LateAttrParsingExperimentalOnly` late parsing is requested, the
  attribute will be late parsed.

The motivation behind this change is to allow the new `counted_by`
attribute (part of `-fbounds-safety`) to support late parsing but
**only** when `-fexperimental-late-parse-attributes` is enabled. This
attribute needs to support late parsing to allow it to refer to fields
later in a struct definition (or function parameters declared later).
However, there isn't a precedent for supporting late attribute parsing
in C so this flag allows the new behavior to exist in Clang but not be
on by default. This behavior was requested as part of the
`-fbounds-safety` RFC process
(https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/68).

This patch doesn't introduce any uses of `LateAttrParseExperimentalExt`.
This will be added for the `counted_by` attribute in a future patch
(#87596). A consequence is the
new behavior added in this patch is not yet testable. Hence, the lack of
tests covering the new behavior.

rdar://125400257
@delcypher delcypher force-pushed the count-on-pointers-and-late-parsing branch 4 times, most recently from 4e6130e to 50e628d Compare May 1, 2024 21:26
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.

For example this previously wasn't allowed:

```
struct BufferTypeDeclAttributePosition {
  size_t count;
  char* buffer __counted_by(count); // Now allowed
}
```

Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:

* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member

This patch also introduces late parsing of the attribute when used
in the declaration attribute position. For example

```
struct BufferTypeDeclAttributePosition {
  char* buffer __counted_by(count); // Now allowed
  size_t count;
}
```

is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`.  The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.

Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:

```
struct BufferTypeAttributePosition {
  size_t count;
  char *__counted_by(count) buffer; // Now allowed
}
```

However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:

```
struct BufferTypeAttributePosition {
  char *__counted_by(count) buffer; // Fails to parse
  size_t count;
}
```

The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.

There are also several other follow up changes that will need to be
addressed in future patches:

* Make late parsing working with anonymous structs (see
  `on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjedts (e.g. parameters, returns types)
  when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g.
for `_builtin_dynamic_object_size` and UBSan's array-bounds checks).

This work is heavily based on a patch originally written by Yeoul Na.

rdar://125400257
@delcypher delcypher force-pushed the count-on-pointers-and-late-parsing branch from 50e628d to a89cca7 Compare May 1, 2024 21:55
@delcypher
Copy link
Contributor

@rapidsna I've put my version of this PR in my own pull request because when I edit the PR summary here it looks like you wrote it which is weird: #90786

@delcypher
Copy link
Contributor

@rapidsna You may want to close this PR but I'll leave this up to you.

@rapidsna rapidsna closed this May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants