Page MenuHomePhabricator

Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute
ClosedPublic

Authored by gargvaibhav64 on Jul 5 2020, 3:07 AM.

Details

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.

Diff Detail

Event Timeline

gargvaibhav64 created this revision.Jul 5 2020, 3:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2020, 3:07 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

@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 edited the summary of this revision. (Show Details)

I incorporated the changes

gargvaibhav64 marked 3 inline comments as done.

Added a new attribute instead of using the previous one

@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 marked 3 inline comments as done.

Incorporated the changes

aaron.ballman added inline comments.Jul 14 2020, 1:02 PM
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?

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.

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.

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 added a subscriber: aaron.ballman.

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 added a comment.EditedJul 26 2020, 1:05 AM

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

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.

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());
}

The test is now working properly.

aaron.ballman added inline comments.Aug 20 2020, 6:56 AM
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 marked an inline comment as done.

Updated the regex to be less-greedy.

aaron.ballman accepted this revision.Aug 21 2020, 7:58 AM

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!

This revision is now accepted and ready to land.Aug 21 2020, 7:58 AM

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: gargvaibhav64@gmail.com

Thanks a lot!

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: gargvaibhav64@gmail.com

Thanks a lot!

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

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?

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.

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.

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.