[clang] Support for read-only types

[malavikasamak]

Summary

Overview

The main goal of this work is to allow developers to express the need to place instances of a class or structure in the read-only part of the program memory. Such a placement is desirable to prevent any further modifications to the instances of a given structure, by leveraging the read-only run time protection.

To achieve this, we are introducing a new attribute that can be attached to any record definition or a declaration. The compiler enforces that every instance of this type can be placed in the read-only segment of the program memory, provided the target triplet supports such a placement. If an instance of a given type bearing this attribute doesn’t satisfy such a placement, the compiler attaches an appropriate warning at suitable program locations. In other words, adding this attribute to a type requires every instance of this type to be a global const, which are placed in the read-only segments for most target triplets. However, this is *not a language feature* and it *need not* be true for *all target triplets*.

Read-only placement of instances

In this subsection, we briefly discuss when an instance generally qualifies to be placed in the read-only part of the program memory.

The instance must satisfy the following:

1. The instance is globally declared and is not allocated on either the heap or stack.
2. The variable declaration is const-qualified.
3. No fields of the instance are mutable. This includes fields directly defined by the type and those inherited from other types.
4. The type defines no explicit non-constexpr constructor for initialization.
5. If another type defines an instance of the type bearing this attribute , then the owning instance should also be eligible to be placed in the read-only memory. In other words, the owning instance is const. qualified global, doesn’t define/inherit mutable fields and has no constructors that force the instance out of read-only memory.
6. Every other type that inherits from this type must also adheres to all the above listed requirements.

Plan

The goal is to build checkers/analyses that can detect the violations of above requirements for a given type and emit warning during compilation. We plan to roll out this features with multiple. In the current patch, we are adding support to attach attributes to record declarations and definitions. The clang semantic analysis checks that every global declaration of this record type is const qualified, otherwise it emits suitable warnings and notes. The current patch relies on the user to ensure the attribute is only attached to appropriate types that already satisfy requirements (1, 3, 4, 5 and 6). However, as stated above we will be adding additional support to ensure they are satisfied by a given type.

Introducing new Attribute: enforce_read_only_placement

The attribute can be attached to either a type declaration or the type definition

*Example usage:*

• struct __*attribute__*(enforce_read_only_placement) Foo;
• struct __*attribute__*(enforce_read_only_placement) Bar { ... };

Warning Messages and Notes emitted:

1. Current patch:
   1. Global variable declaration site without const qualification: (Part of current patch): “Variable of type ‘T’ will not be in the read only program memory.”
   2. Corresponding note attached to the type definition/declaration when a warning is emitted: “Type ‘T’ was declared as a read-only type here.”

2. Future patches:
   1. Allocation on heap (stack): “Instance won't be placed in the read only program memory.”
   2. Mutable field definition: “Instances of type ‘T’ will not be in the read only program memory.”
   3. Explicit non-constexpr constructor definition: “Instances of type ‘T’ will not be in the read only program memory.”
   4. Inheriting from a structure that defines mutable fields.: “Inheriting from a type that can not be in the read only program memory”
   5. Type Q inherits from type T, but Q doesn’t define the attribute.: “‘Q’ not declared as a read-only program memory placeable type.”
   6. Type Q defines a field of type T, but Q doesn’t define the attribute.: “‘Q’ not declared as a read-only program memory placeable type.”

[malavikasamak]

Cosmetic changes.

1. Added new line at the end of 3 files.
2. Removed an extra space from a file.

[malavikasamak]

Added context.

[NoQ]

Nice, I'm glad this is going!

A bit of motivation. The specific struct we really want to put this attribute on is struct IOExternalMethodDispatch that's present in a large portion of macOS drivers (and provided by the KDK). This struct is basically an array of function pointers, traditionally allocated globally, and these function pointers are invoked through interprocess communication (recall that mach is a microkernel). It is extremely important to prevent the attacker from being able to overwrite this struct. Simply putting a const qualifier on such global variable goes a long way as it puts the struct into read-only data segment which is protected at runtime from being overwritten even if undefined behavior occurs.

The problem seems general enough though, as most systems have read-only data segments and there's sometimes similar value in keeping certain data structures there, so I think generalizing it to an attribute is appropriate.

[NoQ]

clang/include/clang/Basic/AttrDocs.td

6778

That's the official catchy term, I believe.

6780

6795–6796

This is documentation for the users, they don't usually think in terms of patches, they usually think in terms of versions. Maybe "The following conditions would also prevent the data to be put into read only segment, but the corresponding warnings are not implemented yet:"?

clang/lib/Sema/SemaDecl.cpp

7390–7391

Not sure about static locals, maybe VD->hasLocalStorage() is more appropriate?

7395

There's some unusual problems with dyn_cast-ing to array types, there's ASTContext::getAsArrayType() and related methods that IIUC should be used instead (they have some comments with respect to why but I don't fully understand them).

7405–7407

I think there are like fifteen different ways how a type needs to be unwrapped, and they can be stacked on top of each other, so it probably makes sense to cover them all at once with type = type->getCanonicalType() at the beginning.

7415–7417

I think this can be done in O(1) with RD->hasAttr<ReadOnlyPlacementAttr>() or something like that.

[xazax.hun]

Hi!

I have some questions:

• Does this affect codegen in any ways? I.e., I'd expect the compiler to already put such structs in read only segments when possible. So are there any scenarios when adding this attribute would affect the binary?
• What do you mean by the type has no explicit constructor? Wouldn't a a type with a user-provided constexpr constructor be a viable candidate to be a read-only type?

[malavikasamak]

Hi Gabor,

1. The attribute doesn't affect CodeGen. It is meant to be a support system to ensure instances of sensitive data types (identified by developer) always satisfy the requirement to be in read-only memory. 
2. Yes. Instances created with constexpr constructors are viable to be in read-only memory. I meant non-constexpr constructors. I will clarify this in the summary.

[NoQ]

In D135851#3857117, @malavikasamak wrote:

1. The attribute doesn't affect CodeGen. It is meant to be a support system to ensure instances of sensitive data types (identified by developer) always satisfy the requirement to be in read-only memory. 

I think that's a great thing to put into user documentation. "The attribute doesn't guarantee that the object will be put into read-only data segment, and it does not instruct the compiler to put it there. It simply warns you if something in your code prevents it from being put there".

[malavikasamak]

Addressed all the comments.

[erichkeane]

This seems like a good start, but I think the rest of the promised warnings would have to be in place before we should commit this. Additionally, we obviously need a slew of SEMA tests.

Additionally: This is intended to be used for the purposes of static-analysis, right? Do we have any static-analysis tools that have dedicated to using this attribute?

clang/lib/Sema/SemaDecl.cpp

329–330

Unrelated change?

[aaron.ballman]

You want to write an annotation on the *type* saying "instances of this should go into a read-only section", but that's not a property of the type, that's a property of a declaration involving that type. I can see the appeal to "if someone makes a declaration that can't be read-only, you should diagnose" but then I stop to consider all the places the type can be used and I'm not certain what should happen. Consider:

struct __attribute__((enforce_read_only_placement)) Foo {
  const int a, b;
};

void func(Foo f); // Do we diagnose this use?
void other(const Foo f); // This one?
Foo druther(); // This one?
constexpr void another(Foo f) {} // This one?
consteval void again(Foo f) {} // This one?

struct S {
  Foo f; // This one?
  int i = sizeof(Foo); // Surely not this one?
};

Would it make sense to use an annotation on the declaration to say "if this specific declaration can't be put in read-only memory, please diagnose" or does that not get you the properties you're looking for?

[malavikasamak]

Apologies about the missing test cases. I had them in the first diff that I uploaded and somehow dropped the file in later diffs.

This diff now contains the test file from the first diff.

[malavikasamak]

Attempt to fix build failure.

[NoQ]

In D135851#3862105, @erichkeane wrote:

I think the rest of the promised warnings would have to be in place before we should commit this.

Hmm, can you elaborate on that? What are the usual requirements for an initial commit of this sort?

Like, what I see is warnings of two different kinds:

• "warning: you're misusing the attribute" (eg., the struct has a mutable field or a non-trivial constructor);
• "warning: you're misusing the object that wears the attribute" (eg. about allocating object on stack or heap).

My impression so far was that the warnings of the first category are somewhat important to get right from the start (at least the most basic constraints) but if warnings from the second category aren't implemented, they simply turn into false negatives. False negatives don't actually hurt anybody, the attribute with false negatives is still more useful than lack of attribute. In many cases it's not even possible to address all false negatives. Wouldn't it make sense to interrupt the initial commit whenever it reaches the point of having no downsides, and address false negatives later?

I'd also like to add that Malavika is on our team, so we'll most likely continue to support the attribute for a while no matter what. I think she also plans to answer the other questions so I'll leave it up to her ^.^

In D135851#3868879, @malavikasamak wrote:

Attempt to fix build failure.

Ok this time it's in libfuzzer, that's clearly not your fault. I think you can ignore them until your patch is ready to commit, but then you can do a few more rebases until it gets green.

[malavikasamak]

This comment was removed by malavikasamak.

[malavikasamak]

Our plan is to first roll out the analysis in the following order:

1. Warn if the attribute is attached to type declarations that prevents read-only placements:
   1. Defining/ inheriting mutable fields
   2. Non-constexpr constructors
2. Warn when an instance will not be placed in read-only program memory:
   1. Global variables and static variables within a method without const qualification (Rolling it out first as we have seen quite a few of such use cases)
   2. Instances created on stack (see above comment for some examples) or allocated on the heap.
3. Warn about missing attribute on types:
   1. Type Q inherits from type T (that bears the attribute), but Q doesn’t define the attribute.
   2. Type Q defines a field of type T (that bear the attribute), but Q doesn’t define the attribute.

[malavikasamak]

Thanks very much for the comment and a list of interesting use cases.

In D135851#3865289 https://reviews.llvm.org/D135851#3865289, @aaron.ballman wrote:

You want to write an annotation on the *type* saying "instances of this should go into a read-only section", but that's not a property of the type, that's a property of a declaration involving that type.

Thank you. Yes, this should be a declaration attribute.

I can see the appeal to "if someone makes a declaration that can't be read-only, you should diagnose" but then I stop to consider all the places the type can be used and I'm not certain what should happen. Consider:

Generally for method definitions, we plan to warn at the declaration site if a method clearly creates an instance on the stack that can be mis-used. If it is not clear whether an instance is created on the stack ( eg., a method bearing constexpr qualifier), we plan to warn at the method invocation sites that create instances on the stack.

struct __attribute__((enforce_read_only_placement)) Foo {
  const int a, b;
};

void func(Foo f); // Do we diagnose this use?

Yes. This always leads to an instance of Foo on the stack. Here, we can warn at the declaration.

void other(const Foo f); // This one?

Yes. This also creates an instance of Foo on the stack, where there is no run time protection. The instance can be modified after using const_cast to get a pointer.

Foo druther(); // This one?

Yes. This always creates a writable instance. So we issue a warning.

constexpr void another(Foo f) {} // This one?

This depends on the invocation. If the LHS of the method invocation is a constexpr then no, otherwise we need to warn at the invocation site.

consteval void again(Foo f) {} // This one?

As far as I understand, this doesn’t seem to create instances on stack. So no warning.

struct S {

Foo f; // This one?
int i = sizeof(Foo); // Surely not this one?

No, as this doesn't create an instance on the stack.

};

[malavikasamak]

Gentle ping.

[xazax.hun]

clang/include/clang/Basic/Attr.td

4101

So as far as I understad, you don't want to support type aliases. I.e.:

struct Foo {};

using ImmutableFoo = [[clang::enforce_read_only_placement]] Foo;

is out of scope.

clang/lib/Sema/SemaDecl.cpp

7388

I'd prefer a local auto variable over a std::function.

Also I think this could be moved closer to its use.

7390

Is this comment relevant?

7394

Do we need recursion? I'd prefer an iterative solution.

7404

Redundant parens?

[malavikasamak]

Addressing comments from the review.

[malavikasamak]

@xazax.hun: Thanks very much for the comments. They are all addressed in the latest diff.

Regarding type aliases: We currently are planning to only allow adding the attribute to a record declaration.

clang/include/clang/Basic/Attr.td

4101

We currently are planning to only allow adding the attribute to a record declaration.

[xazax.hun]

Start to look good, I still have a couple of questions/comments inline.

clang/include/clang/Basic/AttrDocs.td

6777

Since it can be attached to declaration, what would happen in the following scenario:

struct MyType{ int x; };

void foo(MyType t);

struct __attribute__(enforce_read_only_placement) MyType;

Would we diagnose foo even though the attribute was added later? Or are we diagnosing mutable uses AFTER the attribute was introduced?

6786

What is the ConstVarDecl attribute? Are you referring to enforce_read_only_placement?

clang/include/clang/Basic/DiagnosticSemaKinds.td

5679

Other diagnostics in this file seem to start with lower case letters and do not have end of the sentence punctuations.

clang/lib/Sema/SemaDecl.cpp

7399

Could be moved into the isArrayType branch.

[malavikasamak]

clang/include/clang/Basic/AttrDocs.td

6786

Yes. Thank you for catching this! Will fix in the next diff.

clang/include/clang/Basic/DiagnosticSemaKinds.td

5679

Fixed in the new diff.

[malavikasamak]

Addressed the recent comments.

[xazax.hun]

Mostly looks good to me, but there still is a comment that is unaddressed.

clang/include/clang/Basic/AttrDocs.td

6777

I'd like to see a test case like this and in case it does not work I'd like to know whether it is in scope or not. If it is not, let's document this limitation.

[aaron.ballman]

You should also add a release note for the new functionality.

Foo druther(); // This one?

Yes. This always creates a writable instance. So we issue a warning.

Doesn't guaranteed RVO ensure that this doesn't always create a writeable instance (it might be the caller who creates the instance and druther() constructs into that space when returning?

clang/include/clang/Basic/Attr.td

4102

clang/include/clang/Basic/AttrDocs.td

6776–6777

A definition is a declaration, so I think we can make it less awkward by pulling that out. Also, I think "record" is more of an internal term and users are more used to thinking in terms of structures and unions.

6777

The way inheritable attributes work is to allow subsequent redeclarations to add additional information. So for that code example, I'd expect no diagnostic on foo().

6779

6780

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

6786

6788

6789

6791

6796

I'm trying to add some weasel words here to make it clear that a lack of a warning doesn't necessarily mean "everything marked will be in a read-only section".

6800

6803–6809

clang/include/clang/Basic/DiagnosticSemaKinds.td

5679–5681

I think the %0 isn't needed in the note because that note is always attached to the declaration of the type and that should already clearly identify the name of the type.

clang/lib/Sema/SemaDecl.cpp

7381–7385

7392

7395–7396

This should probably move up above the check for local storage and const qualification. Also, type doesn't meet our usual naming conventions but Type is the name of a type. Can you go with something more like VarTy (I'm not tied to that name, just making sure it fits our usual coding style.)

7400

7404

Similar renaming (feel free to pick a better name).

7408–7409

I think this is dead code -- getting the canonical type should strip all sugar off the type, and elaboration is a type of sugar.

7414–7415

The check for hasAttrs() is redundant -- getAttr() already tests that property.

7418

7419–7421

I'm not certain these comments add much value -- the code is reasonably self-explanatory as is.

7422–7424

Shouldn't need to cast to Attr * because it inherits from Attr * already.

[malavikasamak]

clang/include/clang/Basic/AttrDocs.td

6777

Ideally, we would like to attach the warning to all locations that break the read-only memory placement. For the specific case you provided, clang already attaches the "attribute declaration must precede definition" warning (see test case in attr-read-only-placement.cpp, line 44-46), so it is not clear if we should consider cases where declarations follow the definition. Also, we do not yet handle problematic method declarations and I can add this case to the TODO test cases in attr-read-only-placement.cpp.

[malavikasamak]

Addressing the comments in the new diff.

clang/lib/Sema/SemaDecl.cpp

7408–7409

Thank you for pointing this out.

[xazax.hun]

Thanks, the latest version looks good to me.

clang/include/clang/Basic/AttrDocs.td

6777

Sorry, I somehow missed the tests at line 44-46.

clang/lib/Sema/SemaDeclAttr.cpp

7732

Typo.

[aaron.ballman]

Just a few small nits and questions, but you should definitely write a release note for this before landing.

clang/include/clang/Basic/DiagnosticSemaKinds.td

5679–5680

clang/lib/Sema/SemaDecl.cpp

7386–7394

Simplification

clang/test/Sema/attr-read-only-placement.cpp

54

Wouldn't the warning be that C might not be placed in read-only memory because E is not also marked as requiring it?

68

71

Double-checking: the reason this can't be placed in read-only memory is because G can't be placed there due to a mutable member, right? If so, then would this be accepted:

struct __attribute__((enforce_read_only_placement)) Base {
  int i;
};

struct __attribute__((enforce_read_only_placement)) Derived : Base {
  int j;
};

and similarly, is this accepted:

struct __attribute__((enforce_read_only_placement)) Thing {
  int a;
};

struct __attribute__((enforce_read_only_placement)) WrapperThing {
  Thing t;
};

[malavikasamak]

Addressed more comments.

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

7405

clang/lib/Sema/SemaDeclAttr.cpp

7734–7736

No need for that check -- the common attribute handling code will issue a diagnostic if the subject is incorrect.

clang/test/Sema/attr-read-only-placement.cpp

54

I'm still wondering which way this warnings -- is it never going to be possible to compose objects with this attribute together?

71

Still wondering about this as well.

[malavikasamak]

clang/test/Sema/attr-read-only-placement.cpp

54

Looks like I accidentally missed the comments that were added with the in-line changes. Sorry about that.

Compositions in some cases will be possible. For example, inheriting from a base class/struct without the attribute and attaching the attribute to the derived class/struct should be possible, as long as nothing in base class/struct prevents the read-only placement of the derived type. I have added some positive examples to clarify this.

71

Missed this too. Sorry again.

Yes, the above two examples must be accepted. Added a few positive test cases for clarity.

[aaron.ballman]

clang/test/Sema/attr-read-only-placement.cpp

54

Looks like I accidentally missed the comments that were added with the in-line changes. Sorry about that.

No worries, Phabricator is sometimes complex! :-)

Compositions in some cases will be possible. For example, inheriting from a base class/struct without the attribute and attaching the attribute to the derived class/struct should be possible, as long as nothing in base class/struct prevents the read-only placement of the derived type. I have added some positive examples to clarify this.

Thank you, but the comment on this particular case has me confused then. C has no members and E has no members, so there's nothing preventing either from being in read-only memory. So the comment sounds correct in general (we don't have any checking) but incorrect on this specific test case (because even if we had such checking, I'd expect we wouldn't diagnose *that* usage).

I think we can update the comment here to say that this should NOT be diagnosed, and add FIXME to the comments on struct H.

60

Similar here as above. I think you can add a test case like struct H but which wraps struct G instead of inherits from it.

99

101–103

I think other test cases that would be useful are:

struct __attribute__((enforce_read_only_placement)) ConstevalCtor {
  int b;

  consteval ConstevalCtor(int B) : b(B) {} // Fine, right?
};

struct __attribute__((enforce_read_only_placement)) InClassInitializers {
  int b = 12;

  InClassInitializers() = default; // Fine, right?
};

int foo();
struct __attribute__((enforce_read_only_placement)) InClassInitializersBad {
  int b = foo(); // Not fine, right?

  InClassInitializersBad() = default;
};

[malavikasamak]

clang/test/Sema/attr-read-only-placement.cpp

54

You are right that there is nothing preventing E from being in read only memory. The question here we considered is should we even allow the absence of the attribute at the declaration of E?

So we explored two design choices:
a) Emit the warning at the object allocation site for E as C bears the attribute.
b) Skip the diagnosis at the object allocation sites for E and only emit a warning at type E declaration site asking the user to attach the attribute to E. Here, we will require each type to explicitly attach the attribute at the type declaration site, so we can analyze the code for read-only placement violations.

Let’s consider the below example:

int foo();

struct __attribute__((enforce_read_only_placement)) T {int a;};

struct P : T {
  int b; 
  mutable int c;
};

struct Q : T {
   int d = foo();
};

struct R : T {

}; 

T t1{10}; 
P p1{10, 20, 30};
Q q1{10};

struct A : P {};
struct B : A {};

A a1{1, 2, 3};
B b1{4, 5, 6};

Here, choice a will emit a warning at object allocation sites for p1, q1, a1 and b1 as they are not const qualified. In addition, it will also create a warning for type P for defining a mutable field and for type Q for field d initialization.

In contrast, choice b will emit 3 warnings for type P, Q, and R at their type declaration sites warning the user about the missing explicit attribute. Once this attribute is explicitly attached to these types by the user, it will then analyze and warn about any problem in their object allocation sites, field declarations and constructor definitions etc,. Note that, here we also notify the user about type R which has no object instances.

Overall, both approaches will eventually warn about all the code locations that push an object out of read-only memory. The only difference is, choice a presents them all in one step, whereas choice b presents them incrementally.

There are pros and cons for both choices.

Choice a
Pro: Clean code as we can express the need for entire type hierarchy instances to be in read-only memory by simply attaching the attribute to a base type.
Con: Can create warnings for many types at many locations at once, overwhelming/confusing the user. Especially, if the base type is part of a library.

Choice b
Pro: Fewer warnings in the code at each iteration and the warnings are only issued on types that developer explicitly requires to be in the read-only memory.
Con: Every type that needs such protection must bear the attribute. This can make the code a bit cluttered.

[aaron.ballman]

clang/test/Sema/attr-read-only-placement.cpp

54

The question here we considered is should we even allow the absence of the attribute at the declaration of E?

That's a fantastic question to be asking! Thank you very much for the detailed explanation of what you considered, that really helps me as a reviewer. I still have a few code scenarios that may help me develop a better mental model for the design.

What about more complicated class hierarchies? e.g., mix-ins and "holes" in the hierarchy?

// Mix-in to cause a class hierarchy to become read-only
struct  __attribute__((enforce_read_only_placement)) make_type_read_only {};
struct base { int i; };
struct derived : base, make_type_read_only { int j; };

// Middle of the hierarchy missed the attribute.
struct __attribute__((enforce_read_only_placement)) another_base { int i; };
struct another_derived : another_base { int j; };
struct __attribute__((enforce_read_only_placement)) further_derived : another_derived { int k; };

// Bookends of the hierarchy missed the attribute.
struct yet_another_base { int i; };
struct __attribute__((enforce_read_only_placement)) yet_another_derived : yet_another_base { int j; };
struct yet_another_further_derived : yet_another_derived { int k; };

[malavikasamak]

Thanks for the questions.

// Mix-in to cause a class hierarchy to become read-only
struct attribute((enforce_read_only_placement)) make_type_read_only {};
struct base { int i; };
struct derived : base, make_type_read_only { int j; };

We prefer to proceed with choice b, where we warn at the type declaration site. Where, if a base
type bears the attribute but its derived type doesn't, then we'd warn at the declaration site of the
derived type. So for the above example, we will warn at the declaration site for 'derived' type as it
inherits from 'make_type_read_only'.

// Middle of the hierarchy missed the attribute.
struct attribute((enforce_read_only_placement)) another_base { int i; };
struct another_derived : another_base { int j; };
struct attribute((enforce_read_only_placement)) further_derived : another_derived { int k; };

// Bookends of the hierarchy missed the attribute.
struct yet_another_base { int i; };
struct attribute((enforce_read_only_placement)) yet_another_derived : yet_another_base { int j; };
struct yet_another_further_derived : yet_another_derived { int k; };

Going by the same logic as stated above, we'd then warn at the 'another_derived' and 'yet_another_further_derived'
declaration sites, as they also inherit from a attribute bearing types.

[aaron.ballman]

In D135851#3955632, @malavikasamak wrote:

Thanks for the questions.

// Mix-in to cause a class hierarchy to become read-only
struct attribute((enforce_read_only_placement)) make_type_read_only {};
struct base { int i; };
struct derived : base, make_type_read_only { int j; };

We prefer to proceed with choice b, where we warn at the type declaration site. Where, if a base
type bears the attribute but its derived type doesn't, then we'd warn at the declaration site of the
derived type. So for the above example, we will warn at the declaration site for 'derived' type as it
inherits from 'make_type_read_only'.

// Middle of the hierarchy missed the attribute.
struct attribute((enforce_read_only_placement)) another_base { int i; };
struct another_derived : another_base { int j; };
struct attribute((enforce_read_only_placement)) further_derived : another_derived { int k; };

// Bookends of the hierarchy missed the attribute.
struct yet_another_base { int i; };
struct attribute((enforce_read_only_placement)) yet_another_derived : yet_another_base { int j; };
struct yet_another_further_derived : yet_another_derived { int k; };

Going by the same logic as stated above, we'd then warn at the 'another_derived' and 'yet_another_further_derived'
declaration sites, as they also inherit from a attribute bearing types.

Thank you for the discussion, I think that choice b makes sense to me -- it does mean more markings, but it's also more clear as to why you get a diagnostic and how to repair it. Maybe in a follow-up you can consider adding fix-its to add the attribute to an interface which needs it?

clang/test/Sema/attr-read-only-placement.cpp

54

[malavikasamak]

Thank you for the discussion, I think that choice b makes sense to me -- it does mean more markings, but it's also more clear as to why you get a diagnostic and how to repair it.

Absolutely. This choice should be more intuitive to the users in general, as this also makes warnings added at object allocation sites easy to understand and reason about.

Maybe in a follow-up you can consider adding fix-its to add the attribute to an interface which needs it?

Yes, We can have a follow up patch that provides a fixit for the missing attribute warning.

Can I go ahead and land the current patch if there aren't any more questions/concerns?

[aaron.ballman]

Mostly just nits from me, but otherwise LG. I'll leave it to @erichkeane to make the final sign-off.

clang/include/clang/Basic/DiagnosticSemaKinds.td

5679–5680

Wrapped for the usual 80-col limit (clang-format usually botches .td files).

clang/test/Sema/attr-read-only-placement.cpp

7

If using bookmarks like this, it's better to put the expected-note@#1 next to the diagnostic which generates the note instead of putting it up by where the note is issued. Examples below. Same applies elsewhere in the test.

10–13

[malavikasamak]

Addressed comments on .td file width and placement of notes.

[aaron.ballman]

LGTM, thank you for this!

[malavikasamak]

@aaron.ballman: Thanks very much for all the detailed feedback over the past few weeks and for accepting the patch.

@erichkeane: Please let me know if you have any concerns/questions about this patch. If not, I can go ahead and commit this patch.

[jkorous]

Hi @erichkeane,
I am sorry to bother you but in case you are ok with the patch could you please approve?
It sounds like you are taking a break from reviews starting later this week until January.
I'd appreciate if you could unblock us (unless you still have some comments).

[erichkeane]

A couple of minor nits, I think the code you have to do the checking is a little overly complex, but the checking itself is generallyf ine. Else, some nits.

clang/include/clang/Basic/AttrDocs.td

6783

6784

clang/lib/Sema/SemaDecl.cpp

126

extraneous change, please dont commit this.

7393

I'm about 95% sure that this whole section can be replaced by:

VarType = Context.getBaseElementType(VarType);

This would then allow you to handle arrays of pointers (which it seems like you don't do correctly with the 'else if' below.

MIGHT also want a 'get canonical type' somewhere after that as well though.

7399

thinking about it, this whole branch is unnecessary, right? PointerType would just fail the RecordDecl type check below?

clang/lib/Sema/SemaDeclAttr.cpp

8597

clang/test/Sema/attr-read-only-placement.cpp

5

I typically prefer descriptive-ish bookmark names.

[malavikasamak]

Addressed Erich's comments.

clang/lib/Sema/SemaDecl.cpp

7393

Updated the code and it seems to work fine (at least for the test cases) without a second invocation to getCanonicalType(). So, not adding a second invocation for ArrayType.

7399

Yes, you are right.

clang/docs/ReleaseNotes.rst

- Updated the value returned by ``__has_c_attribute(nodiscard)`` to ``202003L``
   supported the ability to specify a message in the attribute, so there were no
   changes to the attribute behavior.
 
 - Updated the value returned by ``__has_c_attribute(fallthrough)`` to ``201910L``
   based on the final date specified by the C2x committee draft. We previously
   used ``201904L`` (the date the proposal was seen by the committee) by mistake.
   There were no other changes to the attribute behavior.
 
+- Introduced a new record declaration attribute ``__attribute__((enforce_read_only_placement))``
+  to support analysis of instances of a given type focused on read-only program 
+  memory placement. It emits a warning if something in the code provably prevents 
+  an instance from a read-only memory placement.
+
 Windows Support
 ---------------
 - For the MinGW driver, added the options ``-mguard=none``, ``-mguard=cf`` and
   ``-mguard=cf-nochecks`` (equivalent to ``/guard:cf-``, ``/guard:cf`` and
   ``/guard:cf,nochecks`` in clang-cl) for enabling Control Flow Guard checks
   and generation of address-taken function table.
 
 AIX Support

clang/include/clang/Basic/Attr.td

def FunctionReturnThunks : InheritableAttr,
   let Spellings = [GCC<"function_return">];
   let Args = [EnumArgument<"ThunkType", "Kind",
     ["keep", "thunk-extern"],
     ["Keep", "Extern"]
   >];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [FunctionReturnThunksDocs];
 }
+def ReadOnlyPlacement : InheritableAttr {
+  let Spellings = [Clang<"enforce_read_only_placement">];
+  let Subjects = SubjectList<[Record]>;
+  let Documentation = [ReadOnlyPlacementDocs];
+}

clang/include/clang/Basic/AttrDocs.td

 The values ``thunk`` and ``thunk-inline`` from GCC are not supported.
 
 The symbol used for ``thunk-extern`` is target specific:
 * X86: ``__x86_return_thunk``
 
 As such, this function attribute is currently only supported on X86 targets.
   }];
 }
+
+def ReadOnlyPlacementDocs : Documentation {
+  let Category = DocCatType;
+  let Content = [{This attribute is attached to a structure, class or union declaration. 
+  When attached to a record declaration/definition, it checks if all instances 
+  of this type can be placed in the read-only data segment of the program. If it 
+  finds an instance that can not be placed in a read-only segment, the compiler 
+  emits a warning at the source location where the type was used.
+
+  Examples:
+  * ``struct __attribute__(enforce_read_only_placement) Foo;``
+  * ``struct __attribute__(enforce_read_only_placement) Bar { ... };``
+
+  Both ``Foo`` and ``Bar`` types have the ``enforce_read_only_placement`` attribute.
+
+  The goal of introducing this attribute is to assist developers with writing secure
+  code. A ``const``-qualified global is generally placed in the read-only section
+  of the memory that has additional run time protection from malicious writes. By 
+  attaching this attribute to a declaration, the developer can express the intent
+  to place all instances of the annotated type in the read-only program memory.
+
+  Note 1: The attribute doesn't guarantee that the object will be placed in the 
+  read-only data segment as it does not instruct the compiler to ensure such 
+  a placement. It emits a warning if something in the code can be proven to prevent 
+  an instance from being placed in the read-only data segment.
+  
+  Note 2: Currently, clang only checks if all global declarations of a given type 'T'
+  are ``const``-qualified. The following conditions would also prevent the data to be 
+  put into read only segment, but the corresponding warnings are not yet implemented.
+
+  1. An instance of type ``T`` is allocated on the heap/stack.
+  2. Type ``T`` defines/inherits a mutable field.
+  3. Type ``T`` defines/inherits non-constexpr constructor(s) for initialization.
+  4. A field of type ``T`` is defined by type ``Q``, which does not bear the
+     ``enforce_read_only_placement`` attribute.
+  5. A type ``Q`` inherits from type ``T`` and it does not have the
+     ``enforce_read_only_placement`` attribute.
+  }];
+}

clang/include/clang/Basic/DiagnosticGroups.td

def PedanticMacros : DiagGroup<"pedantic-macros",
                      RestrictExpansionMacro,
                      FinalMacro]>;
 
 def BranchProtection : DiagGroup<"branch-protection">;
 
 // HLSL diagnostic groups
 // Warnings for HLSL Clang extensions
 def HLSLExtension : DiagGroup<"hlsl-extensions">;
+
+// Warnings and notes related to const_var_decl_type attribute checks
+def ReadOnlyPlacementChecks : DiagGroup<"read-only-types">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_attr_abi_tag_namespace : Warning<
   InGroup<IgnoredAttributes>;
 def err_abi_tag_on_redeclaration : Error<
   "cannot add 'abi_tag' attribute in a redeclaration">;
 def err_new_abi_tag_on_redeclaration : Error<
   "'abi_tag' %0 missing in original declaration">;
 def note_use_ifdef_guards : Note<
   "unguarded header; consider using #ifdef guards or #pragma once">;
 
+def warn_var_decl_not_read_only : Warning<
+  "object of type %0 cannot be placed in read-only memory">,
+  InGroup<ReadOnlyPlacementChecks>;
+def note_enforce_read_only_placement : Note<"type was declared read-only here">;
+
+
 def note_deleted_dtor_no_operator_delete : Note<
   "virtual destructor requires an unambiguous, accessible 'operator delete'">;
 def note_deleted_special_member_class_subobject : Note<
   "%select{default constructor of|copy constructor of|move constructor of|"
   "copy assignment operator of|move assignment operator of|destructor of|"
   "constructor inherited by}0 "
   "%1 is implicitly deleted because "
   "%select{base class %3|%select{||||variant }4field %3}2 "

clang/lib/Sema/SemaDecl.cpp

private:
   bool AllowInvalidDecl;
   bool WantClassName;
   bool AllowTemplates;
   bool AllowNonTemplates;
 };
 
 } // end anonymous namespace
 
+
 /// Determine whether the token kind starts a simple-type-specifier.
 bool Sema::isSimpleTypeSpecifier(tok::TokenKind Kind) const {
   switch (Kind) {
   // FIXME: Take into account the current language when deciding whether a
   // token kind is a valid type specifier
   case tok::kw_short:
   case tok::kw_long:
   case tok::kw___int64:

 /// return the declaration of that type.
 ///
 /// This routine performs ordinary name lookup of the identifier II
 /// within the given scope, with optional C++ scope specifier SS, to
 /// determine whether the name refers to a type. If so, returns an
 /// opaque pointer (actually a QualType) corresponding to that
 /// type. Otherwise, returns NULL.
 ParsedType Sema::getTypeName(const IdentifierInfo &II, SourceLocation NameLoc,
                              Scope *S, CXXScopeSpec *SS, bool isClassName,
                              bool HasTrailingDot, ParsedType ObjectTypePtr,
                              bool IsCtorOrDtorName,
                              bool WantNontrivialTypeSourceInfo,
                              bool IsClassTemplateDeductionContext,
                              ImplicitTypenameContext AllowImplicitTypename,
                              IdentifierInfo **CorrectedII) {
   // FIXME: Consider allowing this outside C++1z mode as an extension.
   bool AllowDeducedTemplate = IsClassTemplateDeductionContext &&
                               getLangOpts().CPlusPlus17 && !IsCtorOrDtorName &&
static void copyAttrFromTypedefToDecl(Sema &S, Decl *D, const TypedefType *TT) {
   const TypedefNameDecl *TND = TT->getDecl();
   if (const auto *Attribute = TND->getAttr<AttrTy>()) {
     AttrTy *Clone = Attribute->clone(S.Context);
     Clone->setInherited(true);
     D->addAttr(Clone);
   }
 }
 
+// This function emits warning and a corresponding note based on the
+// ReadOnlyPlacementAttr attribute. The warning checks that all global variable
+// declarations of an annotated type must be const qualified.
+void emitReadOnlyPlacementAttrWarning(Sema &S, const VarDecl *VD) {
+  QualType VarType = VD->getType().getCanonicalType();
+
+  // Ignore local declarations (for now) and those with const qualification.
+  // TODO: Local variables should not be allowed if their type declaration has
+  // ReadOnlyPlacementAttr attribute. To be handled in follow-up patch.
+  if (!VD || VD->hasLocalStorage() || VD->getType().isConstQualified())
+    return;
+
+  if (VarType->isArrayType()) {
+    // Retrieve element type for array declarations.
+    while (VarType->isArrayType()) {
+      const ArrayType *AT = S.getASTContext().getAsArrayType(VarType);
+      VarType = AT->getElementType();
+    }
+  } else if (const auto *PtrType = dyn_cast<PointerType>(VarType)) {
+    // TODO: We should emit warning for pointers if the type declaration of the
+    // instance they point-to has ReadOnlyPlacementAttr attribute.
+    return;
+  }
+
+  const RecordDecl *RD = VarType->getAsRecordDecl();
+
+  // Check if the record declaration is present and if it has any attributes.
+  if (RD == nullptr)
+    return;
+
+  if (const auto *ConstDecl = RD->getAttr<ReadOnlyPlacementAttr>()) {
+    S.Diag(VD->getLocation(), diag::warn_var_decl_not_read_only) << RD;
+    S.Diag(ConstDecl->getLocation(), diag::note_enforce_read_only_placement);
+    return;
+  }
+}
+
 NamedDecl *Sema::ActOnVariableDeclarator(
     Scope *S, Declarator &D, DeclContext *DC, TypeSourceInfo *TInfo,
     LookupResult &Previous, MultiTemplateParamsArg TemplateParamLists,
     bool &AddToScope, ArrayRef<BindingDecl *> Bindings) {
   QualType R = TInfo->getType();
   DeclarationName Name = GetNameForDeclarator(D).getName();
 
   IdentifierInfo *II = Name.getAsIdentifierInfo();
 
   if (D.isDecompositionDeclarator()) {
     // Take the name of the first declarator as our name for diagnostic
     // purposes.
     auto &Decomp = D.getDecompositionDeclarator();
     if (!Decomp.bindings().empty()) {
       II = Decomp.bindings()[0].Name;
if (NewVD->isInvalidDecl())
       NewTemplate->setInvalidDecl();
     ActOnDocumentableDecl(NewTemplate);
     return NewTemplate;
   }
 
   if (IsMemberSpecialization && !NewVD->isInvalidDecl())
     CompleteMemberSpecialization(NewVD, Previous);
 
+  emitReadOnlyPlacementAttrWarning(*this, NewVD);
+
   return NewVD;
 }
 
 /// Enum describing the %select options in diag::warn_decl_shadow.
 enum ShadowedDeclKind {
   SDK_Local,
   SDK_Global,
   SDK_StaticMember,

clang/lib/Sema/SemaDeclAttr.cpp

static void handleAMDGPUNumVGPRAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   uint32_t NumVGPR = 0;
   Expr *NumVGPRExpr = AL.getArgAsExpr(0);
   if (!checkUInt32Argument(S, AL, NumVGPRExpr, NumVGPR))
     return;
 
   D->addAttr(::new (S.Context) AMDGPUNumVGPRAttr(S.Context, AL, NumVGPR));
 }
 
+static void handleReadOnlyPlacementAttrs(Sema &S, Decl *D,
+                                         const ParsedAttr &AL) {
+  // TODO: Add a warning if attribute added to declarations that can not be
+  // in the read only program memory.
+  D->addAttr(::new (S.Context) ReadOnlyPlacementAttr(S.Context, AL));
+}
+
 static void handleX86ForceAlignArgPointerAttr(Sema &S, Decl *D,
                                               const ParsedAttr &AL) {
   // If we try to apply it to a function pointer, don't warn, but don't
   // do anything, either. It doesn't matter anyway, because there's nothing
   // special about calling a force_align_arg_pointer function.
   const auto *VD = dyn_cast<ValueDecl>(D);
   if (VD && VD->getType()->isFunctionPointerType())
     return;
S.Diag(AL.getLoc(), diag::err_attribute_invalid_on_decl)
         << AL << D->getLocation();
     break;
   case ParsedAttr::AT_Interrupt:
     handleInterruptAttr(S, D, AL);
     break;
   case ParsedAttr::AT_X86ForceAlignArgPointer:
     handleX86ForceAlignArgPointerAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_ReadOnlyPlacement:
+    handleReadOnlyPlacementAttrs(S, D, AL);
+    break;
   case ParsedAttr::AT_DLLExport:
   case ParsedAttr::AT_DLLImport:
     handleDLLAttr(S, D, AL);
     break;
   case ParsedAttr::AT_AMDGPUFlatWorkGroupSize:
     handleAMDGPUFlatWorkGroupSizeAttr(S, D, AL);
     break;
   case ParsedAttr::AT_AMDGPUWavesPerEU:

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: OptimizeNone (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
 // CHECK-NEXT: Owner (SubjectMatchRule_record_not_is_union)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PatchableFunctionEntry (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Pointer (SubjectMatchRule_record_not_is_union)
 // CHECK-NEXT: RandomizeLayout (SubjectMatchRule_record)
+// CHECK-NEXT: ReadOnlyPlacement (SubjectMatchRule_record)
 // CHECK-NEXT: ReleaseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: Restrict (SubjectMatchRule_function)
 // CHECK-NEXT: ReturnTypestate (SubjectMatchRule_function, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
 // CHECK-NEXT: SYCLSpecialClass (SubjectMatchRule_record)

clang/test/Sema/attr-read-only-placement.cpp

+// RUN: %clang_cc1 -Wread-only-types %s -verify -fsyntax-only 
+// RUN: %clang_cc1 -std=c++2a -Wread-only-types %s -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++17 -Wread-only-types %s -verify -fsyntax-only
+
+struct __attribute__((enforce_read_only_placement)) A { // #1
+};
+
+A a1; // expected-warning {{object of type 'A' cannot be placed in read-only memory}}
+      // expected-note@#1 {{type was declared read-only here}}
+const A a2[10]; // no-warning
+A a3[20]; // expected-warning {{object of type 'A' cannot be placed in read-only memory}}
+          // expected-note@#1 {{type was declared read-only here}}
+
+
+
+struct B;
+struct __attribute__((enforce_read_only_placement)) B { //#2
+};
+
+B b1; // expected-warning {{object of type 'B' cannot be placed in read-only memory}}
+      // expected-note@#2 {{type was declared read-only here}}
+const B b2; // no-warning
+const B b3[4]; // no-warning
+B b4[5]; // expected-warning {{object of type 'B' cannot be placed in read-only memory}}
+         // expected-note@#2 {{type was declared read-only here}}
+B b5[5][5]; // expected-warning {{object of type 'B' cannot be placed in read-only memory}}
+            // expected-note@#2 {{type was declared read-only here}}
+B b10[5][5][5]; // expected-warning {{object of type 'B' cannot be placed in read-only memory}}
+                // expected-note@#2 {{type was declared read-only here}}
+
+void method1() {
+    static const B b6;
+    static B b7;// expected-warning {{object of type 'B' cannot be placed in read-only memory}}
+                // expected-note@#2 {{type was declared read-only here}}
+    B b8; // no-warning
+    const B b9; // no-warning
+}
+
+
+struct C;
+struct __attribute__((enforce_read_only_placement)) C; // expected-note {{type was declared read-only here}}
+struct C { // no-note. The note should be attached to the definition/declaration bearing the attribute
+};
+
+C c1; // expected-warning {{object of type 'C' cannot be placed in read-only memory}}
+
+// Cases to be handled by the follow-up patches.
+
+// Attaching and checking the attribute in reverse, where the attribute is attached after the
+// type definition
+struct D;
+struct D { //expected-note{{previous definition is here}}
+};
+struct __attribute__((enforce_read_only_placement)) D; // #3
+                // expected-warning@#3{{attribute declaration must precede definition}}
+
+D d1; // We do not  emit a warning here, as there is another warning for declaring
+      // a type after the definition
+
+
+// Cases where the attribute must be explicitly attached to another type
+// Case 1: Inheriting from a type that has the attribute
+struct E : C { // FIXME: warn the user declarations of type `E`, that extends `C`, won't be
+               // checked for read only placement because `E` is not marked as `C` is.
+};
+
+// Case 2: Declaring a field of the type that has the attribute
+struct F {
+    C c1; // FIXME: warn the user type `F` that wraps type `C` won't be checked for
+          // read only placement
+};
+
+struct BaseWithoutAttribute { 
+    int a;
+};
+
+struct  __attribute__((enforce_read_only_placement)) J : BaseWithoutAttribute { // no-warning
+};
+
+struct __attribute__((enforce_read_only_placement)) BaseWithAttribute {
+    int i;
+};
+
+struct __attribute__((enforce_read_only_placement)) Derived : BaseWithAttribute { // no-warning 
+    int j;
+};
+
+struct __attribute__((enforce_read_only_placement)) WrapperToAttributeInstance { // no-warning
+    BaseWithAttribute b;
+};
+
+struct __attribute__((enforce_read_only_placement)) WrapperToNoAttributeInstance { // no-warning
+    BaseWithoutAttribute b;
+};
+
+// Cases where the const qualification doesn't ensure read-only memory placement
+// of an instance.
+
+// Case 1: The type defines/inherits mutable data members
+struct __attribute__((enforce_read_only_placement)) G {
+    mutable int x; // FIXME: warn the user type `G` won't be placed in the read only program memory
+};
+
+struct __attribute__((enforce_read_only_placement)) H : public G { // FIXME: Warn the user type `H`
+                                                // won't be placed in the read only program memory
+};
+
+struct __attribute__((enforce_read_only_placement)) K { // FIXME : Warn the user type `K` w on't be
+                                                // placed in the read only program memory
+    G g;
+};
+
+
+// Case 2: The type has a constructor that makes its fields modifiable
+struct  __attribute__((enforce_read_only_placement)) L {
+    int b;
+    L(int val) { // FIXME: warn the user type `L` won't be placed in the read only program memory
+      b = val;
+    }
+};
+
+struct __attribute__((enforce_read_only_placement)) ConstInClassInitializers { // no-warning
+  int b = 12;
+
+  ConstInClassInitializers() = default;
+};
+
+int foo();
+struct __attribute__((enforce_read_only_placement)) NonConstInClassInitializers {
+  int b = foo(); // FIXME: warn the user type `NonConstInClassInitializers` won't be placed
+                 // in the read only program memory
+
+  NonConstInClassInitializers() = default;
+};
+
+#if (__cplusplus >= 202002L)
+struct __attribute__((enforce_read_only_placement)) ConstevalCtor {
+  int b;
+
+  consteval ConstevalCtor(int B) : b(B) {} // no-warning
+};
+#endif
+
+#if (__cplusplus >= 201103L)
+struct __attribute__((enforce_read_only_placement)) ConstExprCtor { // no-warning
+  int b;
+
+  constexpr ConstExprCtor(int B) : b(B) {}
+};
+
+constexpr ConstExprCtor cec1(10); // no-warning
+
+#endif
+
+// Cases where an object is allocated on the heap or on the stack
+C *c2 = new C; // FIXME: warn the user this instance of 'C' won't be placed in the read only program memory
+
+void func1(C c); // FIXME: warn the user the instance of 'C' won't be placed in the read only program memory
+
+void func2(const C c); // FIXME: warn the user the instance of 'C' won't be placed in the read
+                       // only program memory
+
+C func3(); // FIXME: warn the user the instance of 'C' won't be placed in the read only program memory
+
+void func4() {
+    C c; // FIXME: warn the user the instance of 'C' won't be placed in the read only program memory
+}
+
+#if (__cplusplus >= 202002L)
+consteval void func4(C c); // no-warning
+#endif