Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

[gargvaibhav64]

Summary

This commit teaches ASTDeclReader::attachPreviousDecl to successfully merge two Decl's when one contains an inheritable attribute like the MSInheritanceAttr.
Usually, attributes that are needed to be present along the redeclaration chain are attached during ASTReading from ASTDeclReader::attachPreviousDecl, but no such thing is done for inheritable attributes. Currently, only the logic for merging MSInheritanceAttr is provided.

[rsmith]

@aaron.ballman We will need to do something like this in general, but I'm not sure it's going to be as easy as just inheriting the attribute in the general case. What do you think?

clang/lib/Serialization/ASTReaderDecl.cpp

3540

I think it would be more consistent to clone the attribute here (and mark it as inherited) rather than attaching the same attribute to multiple declarations.

3544

I think we should only propagate attributes forwards along redeclaration chains. Is this single-step-backwards propagation necessary?

3709

Please add a FIXME to do this is general.

[gargvaibhav64]

I incorporated the changes

[gargvaibhav64]

Added a new attribute instead of using the previous one

[aaron.ballman]

In D83174#2137133, @rsmith wrote:

@aaron.ballman We will need to do something like this in general, but I'm not sure it's going to be as easy as just inheriting the attribute in the general case. What do you think?

I agree that we're going to need a more general solution at some point. We do this automatically in mergeDeclAttributes() with:

else if (Attr->shouldInheritEvenIfAlreadyPresent() || !DeclHasAttr(D, Attr))
  NewAttr = cast<InheritableAttr>(Attr->clone(S.Context));

for the general case (and similar for merging parameter attributes in mergeParamDeclAttributes(), but we have custom merging logic for the other cases. However, the custom merging logic is usually for diagnosing bad combinations of attributes, which I don't think we need to handle in this case, do we?

clang/lib/Serialization/ASTReaderDecl.cpp

3544

Don't do hasAttr followed by getAttr (that duplicates work); also, you don't need to dyn_cast<> the call to getAttr. How about:

const auto *IA = Previous->getAttr<MSInheritanceAttr>();
if (IA && Previous->hasAttr<MSInheritanceAttr>()) {
  ...
}

3546

Rather than do it this way, how about: NewAttr = cast<InheritableAttr>(IA->clone(Context));

clang/test/Modules/Inputs/inherit-attribute/a.h

11

Please add a newline to the end of the file. (Same for the other files.)

[gargvaibhav64]

Incorporated the changes

[aaron.ballman]

clang/lib/Serialization/ASTReaderDecl.cpp

3544

I'm really sorry but I had a think-o in my suggestion. What I really meant was:

const auto *IA = Previous->getAttr<MSInheritanceAttr>();
if (IA && !D->hasAttr<MSInheritanceAttr>()) {
  ...
}

That said, I'm surprised you're not getting a test failure thanks to implementing my mistake. Were the tests failing for you, or are we missing test coverage?

[gargvaibhav64]

The tests weren't failing for me. So, we are possibly missing test coverage. I have made the change now.

Also, I would like to add that the current test present in this diff does not fail in the existing system but it was the best I and Vassil could come up with to replicate our problem.

[aaron.ballman]

In D83174#2155143, @gargvaibhav64 wrote:

The tests weren't failing for me. So, we are possibly missing test coverage. I have made the change now.

Thanks for making the change.

Also, I would like to add that the current test present in this diff does not fail in the existing system but it was the best I and Vassil could come up with to replicate our problem.

Is there a way to run something like creduce to come up with a test case that does replicate the issue? I'd like to ensure we have a test case that fails without the patch applied so that we can be sure we don't regress the behavior.

[v.g.vassilev]

Also, I would like to add that the current test present in this diff does not fail in the existing system but it was the best I and Vassil could come up with to replicate our problem.

Is there a way to run something like creduce to come up with a test case that does replicate the issue? I'd like to ensure we have a test case that fails without the patch applied so that we can be sure we don't regress the behavior.

Not easily. That bug comes from internal infrastructure which uses clang's API and happens when calling directly the mangler. Running creduce would require failure to be reproducible with bare clang. I was hoping @rsmith had something up his sleeve.

[aaron.ballman]

In D83174#2156454, @v.g.vassilev wrote:

Also, I would like to add that the current test present in this diff does not fail in the existing system but it was the best I and Vassil could come up with to replicate our problem.

Is there a way to run something like creduce to come up with a test case that does replicate the issue? I'd like to ensure we have a test case that fails without the patch applied so that we can be sure we don't regress the behavior.

Not easily. That bug comes from internal infrastructure which uses clang's API and happens when calling directly the mangler. Running creduce would require failure to be reproducible with bare clang. I was hoping @rsmith had something up his sleeve.

Drat. While the code looks reasonable, this makes me less confident in the fix. I'm adding some more reviewers who have touched the AST serialization and modules before in case they also have ideas for test cases.

[gargvaibhav64]

Hi everyone, are there any more changes required on this review?

[aaron.ballman]

In D83174#2174294, @gargvaibhav64 wrote:

Hi everyone, are there any more changes required on this review?

FWIW, I'm holding my LG until there's a test case which covers the changes.

[martong]

In D83174#2158275, @aaron.ballman wrote:

In D83174#2156454, @v.g.vassilev wrote:

Also, I would like to add that the current test present in this diff does not fail in the existing system but it was the best I and Vassil could come up with to replicate our problem.

Is there a way to run something like creduce to come up with a test case that does replicate the issue? I'd like to ensure we have a test case that fails without the patch applied so that we can be sure we don't regress the behavior.

Not easily. That bug comes from internal infrastructure which uses clang's API and happens when calling directly the mangler. Running creduce would require failure to be reproducible with bare clang. I was hoping @rsmith had something up his sleeve.

Drat. While the code looks reasonable, this makes me less confident in the fix. I'm adding some more reviewers who have touched the AST serialization and modules before in case they also have ideas for test cases.

I had a patch a few months ago for ASTImporter that tries to properly handle inherited attributes (D68634). That patch had not made it to the upstream, because that's not generic enough, it handles only 2 or 3 attributes. Nevertheless, some of the unit tests might be inspiring and we could reuse them here. E.g. in the below code replace "import" with "deserialization":

TEST_P(ImportAttrs, ImportShouldInheritExistingInheritableAttr) {
  Decl *ToTU = getToTuDecl(
      R"(
      void f() __attribute__((analyzer_noreturn));
      void f();
      )",
      Lang_C);
  Decl *FromTU = getTuDecl(
      R"(
      void f();
      )",
      Lang_C, "input0.c");

  auto *From = FirstDeclMatcher<FunctionDecl>().match(
      FromTU, functionDecl(hasName("f")));

  auto *To0 =
      FirstDeclMatcher<FunctionDecl>().match(ToTU, functionDecl(hasName("f")));
  ASSERT_TRUE(To0->hasAttrs());
  ASSERT_TRUE(To0->getAttr<AnalyzerNoReturnAttr>());
  auto *To1 =
      LastDeclMatcher<FunctionDecl>().match(ToTU, functionDecl(hasName("f")));
  ASSERT_TRUE(To1->hasAttrs());
  ASSERT_TRUE(To1->getAttr<AnalyzerNoReturnAttr>());
  ASSERT_TRUE(To1->getAttr<AnalyzerNoReturnAttr>()->isInherited());

  // Should have an Inherited attribute.
  auto *Imported = Import(From, Lang_C);
  EXPECT_TRUE(Imported->hasAttrs());
  ASSERT_TRUE(Imported->getAttr<AnalyzerNoReturnAttr>());
  EXPECT_TRUE(Imported->getAttr<AnalyzerNoReturnAttr>()->isInherited());
}

[gargvaibhav64]

The test is now working properly.

[aaron.ballman]

clang/test/Modules/Inputs/inherit-attribute/b.h

4

Does this forward declaration impact the test? I can't quite tell whether this exists to test that the attribute is attached to the non-defining redeclaration or not. (One way to make it more clear would be to have a less greedy regex in inherit-attribute.cpp so we can see the source location where the declarations are coming from.)

clang/test/Modules/inherit-attribute.cpp

4

You can drop the -verify and // expected-no-diagnostics as they don't add anything to the test coverage.

[gargvaibhav64]

Updated the regex to be less-greedy.

[aaron.ballman]

LGTM, thank you for the fix! Do you need someone to commit on your behalf? If so, please be sure you're fine with the license agreement and let us know what name and email address you would like associated with the commit. Thanks!

[gargvaibhav64]

In D83174#2230546, @aaron.ballman wrote:

LGTM, thank you for the fix! Do you need someone to commit on your behalf? If so, please be sure you're fine with the license agreement and let us know what name and email address you would like associated with the commit. Thanks!

Hi,
Yes, I am fine with the license agreement.
Name: Vaibhav Garg
E-Mail: [e-mail]

Thanks a lot!

[aaron.ballman]

In D83174#2230616, @gargvaibhav64 wrote:

In D83174#2230546, @aaron.ballman wrote:

LGTM, thank you for the fix! Do you need someone to commit on your behalf? If so, please be sure you're fine with the license agreement and let us know what name and email address you would like associated with the commit. Thanks!

Hi,
Yes, I am fine with the license agreement.
Name: Vaibhav Garg
E-Mail: [e-mail]

Thanks a lot!

Any time, thank you again for the patch! I've commit on your behalf in 7a527f17776be78ec44b88e82b39afb65fc148e4.

[aaron.ballman]

Unfortunately, I had to revert the change as it was causing some buildbots to fail. I reverted in 58c305f466d1f78adb10e7295b9bc9fc192a6e09. Some failures include:

http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/20294
http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/13600

I'm not certain why b.h is not being found by those builders. Do you mind investigating?

[JamesNagurne]

In D83174#2230763, @aaron.ballman wrote:

Unfortunately, I had to revert the change as it was causing some buildbots to fail. I reverted in 58c305f466d1f78adb10e7295b9bc9fc192a6e09. Some failures include:

http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/20294
http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/13600

I'm not certain why b.h is not being found by those builders. Do you mind investigating?

I suspect the failures are due to the Windows-style path in the -I option.
I recall lit being able to handle such a thing, but my memory might be a bit spotty on its command-line emulation capabilities.

[gargvaibhav64]

I updated the -triple option to x86_64-pc-windows-msvc (as it was more consistent with current tests). I also updated the path to Unix style in the -I option.

[aaron.ballman]

In D83174#2230795, @gargvaibhav64 wrote:

I updated the -triple option to x86_64-pc-windows-msvc (as it was more consistent with current tests). I also updated the path to Unix style in the -I option.

Thank you for the fixes, I've re-landed in aca191cce1c4dbab28a65cfe4caa6348e698a2b3.

[v.g.vassilev]

Landed in aca191cce1c4dbab28a65cfe4caa6348e698a2b3

clang/lib/Serialization/ASTReaderDecl.cpp

void AddLazySpecializations(T *D,
       LazySpecializations = Result;
     }
 
     template <typename DeclT>
     static Decl *getMostRecentDeclImpl(Redeclarable<DeclT> *D);
     static Decl *getMostRecentDeclImpl(...);
     static Decl *getMostRecentDecl(Decl *D);
 
+    static void mergeInheritableAttributes(ASTReader &Reader, Decl *D,
+                                           Decl *Previous);
+
     template <typename DeclT>
     static void attachPreviousDeclImpl(ASTReader &Reader,
                                        Redeclarable<DeclT> *D, Decl *Previous,
                                        Decl *Canon);
     static void attachPreviousDeclImpl(ASTReader &Reader, ...);
     static void attachPreviousDecl(ASTReader &Reader, Decl *D, Decl *Previous,
                                    Decl *Canon);
 
#include "clang/AST/DeclNodes.inc"
   }
   llvm_unreachable("unknown decl kind");
 }
 
 Decl *ASTReader::getMostRecentExistingDecl(Decl *D) {
   return ASTDeclReader::getMostRecentDecl(D->getCanonicalDecl());
 }
 
+void ASTDeclReader::mergeInheritableAttributes(ASTReader &Reader, Decl *D,
+                                               Decl *Previous) {
+  InheritableAttr *NewAttr = nullptr;
+  ASTContext &Context = Reader.getContext();
+  const auto *IA = Previous->getAttr<MSInheritanceAttr>();
+
+  if (IA && !D->hasAttr<MSInheritanceAttr>()) {
+    NewAttr = cast<InheritableAttr>(IA->clone(Context));
+    NewAttr->setInherited(true);
+    D->addAttr(NewAttr);
+  }
+}
+
 template<typename DeclT>
 void ASTDeclReader::attachPreviousDeclImpl(ASTReader &Reader,
                                            Redeclarable<DeclT> *D,
                                            Decl *Previous, Decl *Canon) {
   D->RedeclLink.setPrevious(cast<DeclT>(Previous));
   D->First = cast<DeclT>(Previous)->First;
 }
 
D->IdentifierNamespace |=
       Previous->IdentifierNamespace &
       (Decl::IDNS_Ordinary | Decl::IDNS_Tag | Decl::IDNS_Type);
 
   // If the declaration declares a template, it may inherit default arguments
   // from the previous declaration.
   if (auto *TD = dyn_cast<TemplateDecl>(D))
     inheritDefaultTemplateArguments(Reader.getContext(),
                                     cast<TemplateDecl>(Previous), TD);
+
+  // If any of the declaration in the chain contains an Inheritable attribute,
+  // it needs to be added to all the declarations in the redeclarable chain.
+  // FIXME: Only the logic of merging MSInheritableAttr is present, it should
+  // be extended for all inheritable attributes.
+  mergeInheritableAttributes(Reader, D, Previous);
 }
 
 template<typename DeclT>
 void ASTDeclReader::attachLatestDeclImpl(Redeclarable<DeclT> *D, Decl *Latest) {
   D->RedeclLink.setLatest(cast<DeclT>(Latest));
 }
 
 void ASTDeclReader::attachLatestDeclImpl(...) {

clang/test/Modules/Inputs/inherit-attribute/a.h

+#ifndef FOO
+#define FOO
+
+class Foo {
+public:
+  void step(int v);
+  Foo();
+};
+
+#endif

clang/test/Modules/Inputs/inherit-attribute/b.h

+#include "a.h"
+
+void bar() {
+  &Foo::step;
+}

clang/test/Modules/Inputs/inherit-attribute/c.h

+#include "a.h"

clang/test/Modules/Inputs/inherit-attribute/module.modulemap

+module "b" { header "b.h" }
+
+module "c" { header "c.h" }

clang/test/Modules/inherit-attribute.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -triple x86_64-pc-windows-msvc -I%S/Inputs/inherit-attribute -fmodules-cache-path=%t \
+// RUN: -fimplicit-module-maps -fmodules-local-submodule-visibility %s -ast-dump-all \
+// RUN: | FileCheck %s
+
+#include "b.h"
+#include "c.h"
+
+class Foo;
+
+Foo f;
+
+// CHECK:   CXXRecordDecl {{.*}} imported in b {{.*}} Foo
+// CHECK:   MSInheritanceAttr {{[^()]*$}}
+
+// CHECK:   CXXRecordDecl {{.*}} prev {{.*}} imported in c {{.*}} Foo
+// CHECK:   MSInheritanceAttr {{.*}} Inherited {{[^()]*$}}
+
+// CHECK:   CXXRecordDecl {{.*}} <line:9:1, col:7> col:7 referenced class Foo
+// CHECK:   MSInheritanceAttr {{.*}} Inherited {{[^()]*$}}