[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D83174#2158275 <https://reviews.llvm.org/D83174#2158275>, @aaron.ballman wrote:

> In D83174#2156454 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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());
  }


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83174/new/

https://reviews.llvm.org/D83174