[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-11-14 Thread Vassil Vassilev via Phabricator via cfe-commits 
v.g.vassilev closed this revision.
v.g.vassilev added a comment.

Landed in  aca191cce1c4dbab28a65cfe4caa6348e698a2b3 



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

https://reviews.llvm.org/D83174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-08-21 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

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 
.


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

https://reviews.llvm.org/D83174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-08-21 Thread Vaibhav Garg via Phabricator via cfe-commits 
gargvaibhav64 updated this revision to Diff 287060.
gargvaibhav64 added a comment.

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.


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

https://reviews.llvm.org/D83174

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/inherit-attribute/a.h
  clang/test/Modules/Inputs/inherit-attribute/b.h
  clang/test/Modules/Inputs/inherit-attribute/c.h
  clang/test/Modules/Inputs/inherit-attribute/module.modulemap
  clang/test/Modules/inherit-attribute.cpp

Index: clang/test/Modules/inherit-attribute.cpp
===
--- /dev/null
+++ clang/test/Modules/inherit-attribute.cpp
@@ -0,0 +1,20 @@
+// 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 {{.*}}  col:7 referenced class Foo
+// CHECK:   MSInheritanceAttr {{.*}} Inherited {{[^()]*$}}
Index: clang/test/Modules/Inputs/inherit-attribute/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/module.modulemap
@@ -0,0 +1,3 @@
+module "b" { header "b.h" }
+
+module "c" { header "c.h" }
Index: clang/test/Modules/Inputs/inherit-attribute/c.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/c.h
@@ -0,0 +1 @@
+#include "a.h"
Index: clang/test/Modules/Inputs/inherit-attribute/b.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/b.h
@@ -0,0 +1,5 @@
+#include "a.h"
+
+void bar() {
+  ::step;
+}
Index: clang/test/Modules/Inputs/inherit-attribute/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/a.h
@@ -0,0 +1,10 @@
+#ifndef FOO
+#define FOO
+
+class Foo {
+public:
+  void step(int v);
+  Foo();
+};
+
+#endif
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -281,6 +281,9 @@
 static Decl *getMostRecentDeclImpl(...);
 static Decl *getMostRecentDecl(Decl *D);
 
+static void mergeInheritableAttributes(ASTReader , Decl *D,
+   Decl *Previous);
+
 template 
 static void attachPreviousDeclImpl(ASTReader ,
Redeclarable *D, Decl *Previous,
@@ -3531,6 +3534,19 @@
   return ASTDeclReader::getMostRecentDecl(D->getCanonicalDecl());
 }
 
+void ASTDeclReader::mergeInheritableAttributes(ASTReader , Decl *D,
+   Decl *Previous) {
+  InheritableAttr *NewAttr = nullptr;
+  ASTContext  = Reader.getContext();
+  const auto *IA = Previous->getAttr();
+
+  if (IA && !D->hasAttr()) {
+NewAttr = cast(IA->clone(Context));
+NewAttr->setInherited(true);
+D->addAttr(NewAttr);
+  }
+}
+
 template
 void ASTDeclReader::attachPreviousDeclImpl(ASTReader ,
Redeclarable *D,
@@ -3689,6 +3705,12 @@
   if (auto *TD = dyn_cast(D))
 inheritDefaultTemplateArguments(Reader.getContext(),
 cast(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-08-21 Thread James Nagurne via Phabricator via cfe-commits 
JamesNagurne added a comment.

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.


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

https://reviews.llvm.org/D83174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-08-21 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

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?


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

https://reviews.llvm.org/D83174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-08-21 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

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: gargvaibha...@gmail.com
>
> Thanks a lot!

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


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

https://reviews.llvm.org/D83174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-08-21 Thread Vaibhav Garg via Phabricator via cfe-commits 
gargvaibhav64 added a comment.

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

Thanks a lot!


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

https://reviews.llvm.org/D83174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-08-21 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

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!


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

https://reviews.llvm.org/D83174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-08-21 Thread Vaibhav Garg via Phabricator via cfe-commits 
gargvaibhav64 updated this revision to Diff 287004.
gargvaibhav64 marked an inline comment as done.
gargvaibhav64 added a comment.

Updated the regex to be less-greedy.


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

https://reviews.llvm.org/D83174

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/inherit-attribute/a.h
  clang/test/Modules/Inputs/inherit-attribute/b.h
  clang/test/Modules/Inputs/inherit-attribute/c.h
  clang/test/Modules/Inputs/inherit-attribute/module.modulemap
  clang/test/Modules/inherit-attribute.cpp

Index: clang/test/Modules/inherit-attribute.cpp
===
--- /dev/null
+++ clang/test/Modules/inherit-attribute.cpp
@@ -0,0 +1,20 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -triple x86_64-pc-windows-msvc-unknown -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 {{.*}}  col:7 referenced class Foo
+// CHECK:   MSInheritanceAttr {{.*}} Inherited {{[^()]*$}}
Index: clang/test/Modules/Inputs/inherit-attribute/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/module.modulemap
@@ -0,0 +1,3 @@
+module "b" { header "b.h" }
+
+module "c" { header "c.h" }
Index: clang/test/Modules/Inputs/inherit-attribute/c.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/c.h
@@ -0,0 +1 @@
+#include "a.h"
Index: clang/test/Modules/Inputs/inherit-attribute/b.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/b.h
@@ -0,0 +1,5 @@
+#include "a.h"
+
+void bar() {
+  ::step;
+}
Index: clang/test/Modules/Inputs/inherit-attribute/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/a.h
@@ -0,0 +1,10 @@
+#ifndef FOO
+#define FOO
+
+class Foo {
+public:
+  void step(int v);
+  Foo();
+};
+
+#endif
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -281,6 +281,9 @@
 static Decl *getMostRecentDeclImpl(...);
 static Decl *getMostRecentDecl(Decl *D);
 
+static void mergeInheritableAttributes(ASTReader , Decl *D,
+   Decl *Previous);
+
 template 
 static void attachPreviousDeclImpl(ASTReader ,
Redeclarable *D, Decl *Previous,
@@ -3531,6 +3534,19 @@
   return ASTDeclReader::getMostRecentDecl(D->getCanonicalDecl());
 }
 
+void ASTDeclReader::mergeInheritableAttributes(ASTReader , Decl *D,
+   Decl *Previous) {
+  InheritableAttr *NewAttr = nullptr;
+  ASTContext  = Reader.getContext();
+  const auto *IA = Previous->getAttr();
+
+  if (IA && !D->hasAttr()) {
+NewAttr = cast(IA->clone(Context));
+NewAttr->setInherited(true);
+D->addAttr(NewAttr);
+  }
+}
+
 template
 void ASTDeclReader::attachPreviousDeclImpl(ASTReader ,
Redeclarable *D,
@@ -3689,6 +3705,12 @@
   if (auto *TD = dyn_cast(D))
 inheritDefaultTemplateArguments(Reader.getContext(),
 cast(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-08-20 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/test/Modules/Inputs/inherit-attribute/b.h:3
+
+class Foo;
+

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.)



Comment at: clang/test/Modules/inherit-attribute.cpp:3
+// RUN: %clang_cc1 -fmodules -triple x86_64-pc-windows-msvc-unknown 
-I%S\Inputs\inherit-attribute -fmodules-cache-path=%t \
+// RUN: -fimplicit-module-maps -fmodules-local-submodule-visibility -verify %s 
-ast-dump-all \
+// RUN: | FileCheck %s

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


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

https://reviews.llvm.org/D83174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-08-20 Thread Vaibhav Garg via Phabricator via cfe-commits 
gargvaibhav64 updated this revision to Diff 286786.
gargvaibhav64 added a comment.

The test is now working properly.


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

https://reviews.llvm.org/D83174

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/inherit-attribute/a.h
  clang/test/Modules/Inputs/inherit-attribute/b.h
  clang/test/Modules/Inputs/inherit-attribute/c.h
  clang/test/Modules/Inputs/inherit-attribute/module.modulemap
  clang/test/Modules/inherit-attribute.cpp

Index: clang/test/Modules/inherit-attribute.cpp
===
--- /dev/null
+++ clang/test/Modules/inherit-attribute.cpp
@@ -0,0 +1,22 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -triple x86_64-pc-windows-msvc-unknown -I%S\Inputs\inherit-attribute -fmodules-cache-path=%t \
+// RUN: -fimplicit-module-maps -fmodules-local-submodule-visibility -verify %s -ast-dump-all \
+// RUN: | FileCheck %s
+
+#include "b.h"
+#include "c.h"
+
+class Foo;
+
+Foo f;
+
+// CHECK:   CXXRecordDecl{{.*}}prev{{.*}}Foo
+// CHECK:   {{.*}}`-MSInheritanceAttr{{[^()]*$}}
+
+// CHECK:   CXXRecordDecl{{.*}}prev{{.*}}Foo
+// CHECK:   {{.*}}-MSInheritanceAttr{{[^()]*$}}
+
+// CHECK:   CXXRecordDecl{{.*}}prev{{.*}}Foo
+// CHECK:   {{.*}}`-MSInheritanceAttr{{[^()]*$}}
+
+// expected-no-diagnostics
Index: clang/test/Modules/Inputs/inherit-attribute/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/module.modulemap
@@ -0,0 +1,3 @@
+module "b" { header "b.h" }
+
+module "c" { header "c.h" }
Index: clang/test/Modules/Inputs/inherit-attribute/c.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/c.h
@@ -0,0 +1 @@
+#include "a.h"
Index: clang/test/Modules/Inputs/inherit-attribute/b.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/b.h
@@ -0,0 +1,7 @@
+#include "a.h"
+
+class Foo;
+
+void bar() {
+  ::step;
+}
Index: clang/test/Modules/Inputs/inherit-attribute/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/a.h
@@ -0,0 +1,10 @@
+#ifndef FOO
+#define FOO
+
+class Foo {
+public:
+  void step(int v);
+  Foo();
+};
+
+#endif
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -281,6 +281,9 @@
 static Decl *getMostRecentDeclImpl(...);
 static Decl *getMostRecentDecl(Decl *D);
 
+static void mergeInheritableAttributes(ASTReader , Decl *D,
+   Decl *Previous);
+
 template 
 static void attachPreviousDeclImpl(ASTReader ,
Redeclarable *D, Decl *Previous,
@@ -3531,6 +3534,19 @@
   return ASTDeclReader::getMostRecentDecl(D->getCanonicalDecl());
 }
 
+void ASTDeclReader::mergeInheritableAttributes(ASTReader , Decl *D,
+   Decl *Previous) {
+  InheritableAttr *NewAttr = nullptr;
+  ASTContext  = Reader.getContext();
+  const auto *IA = Previous->getAttr();
+
+  if (IA && !D->hasAttr()) {
+NewAttr = cast(IA->clone(Context));
+NewAttr->setInherited(true);
+D->addAttr(NewAttr);
+  }
+}
+
 template
 void ASTDeclReader::attachPreviousDeclImpl(ASTReader ,
Redeclarable *D,
@@ -3689,6 +3705,12 @@
   if (auto *TD = dyn_cast(D))
 inheritDefaultTemplateArguments(Reader.getContext(),
 cast(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-27 Thread Gabor Marton via Phabricator via cfe-commits 
martong added a comment.

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().match(
FromTU, functionDecl(hasName("f")));
  
auto *To0 =
FirstDeclMatcher().match(ToTU, 
functionDecl(hasName("f")));
ASSERT_TRUE(To0->hasAttrs());
ASSERT_TRUE(To0->getAttr());
auto *To1 =
LastDeclMatcher().match(ToTU, functionDecl(hasName("f")));
ASSERT_TRUE(To1->hasAttrs());
ASSERT_TRUE(To1->getAttr());
ASSERT_TRUE(To1->getAttr()->isInherited());
  
// Should have an Inherited attribute.
auto *Imported = Import(From, Lang_C);
EXPECT_TRUE(Imported->hasAttrs());
ASSERT_TRUE(Imported->getAttr());
EXPECT_TRUE(Imported->getAttr()->isInherited());
  }


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

https://reviews.llvm.org/D83174



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-26 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D83174



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-26 Thread Vaibhav Garg via Phabricator via cfe-commits 
gargvaibhav64 added a comment.

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


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

https://reviews.llvm.org/D83174



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-17 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added reviewers: aaron.ballman, riccibruno, martong.
aaron.ballman added a comment.
Herald added a subscriber: rnkovacs.

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.


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

https://reviews.llvm.org/D83174



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-16 Thread Vassil Vassilev via Phabricator via cfe-commits 
v.g.vassilev removed a reviewer: aaron.ballman.
v.g.vassilev added a comment.



>> 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.


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

https://reviews.llvm.org/D83174



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-16 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

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.


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

https://reviews.llvm.org/D83174



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-16 Thread Vaibhav Garg via Phabricator via cfe-commits 
gargvaibhav64 updated this revision to Diff 278399.
gargvaibhav64 added a comment.

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.


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

https://reviews.llvm.org/D83174

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/inherit-attribute/a.h
  clang/test/Modules/Inputs/inherit-attribute/b.h
  clang/test/Modules/Inputs/inherit-attribute/c.h
  clang/test/Modules/Inputs/inherit-attribute/module.modulemap
  clang/test/Modules/inherit-attribute.cpp

Index: clang/test/Modules/inherit-attribute.cpp
===
--- /dev/null
+++ clang/test/Modules/inherit-attribute.cpp
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -ast-dump -I%S/Inputs/inherit-attribute -fmodules-cache-path=%t -fimplicit-module-maps -verify %s -fmodules-local-submodule-visibility
+
+#include "b.h"
+#include "c.h"
+
+class Foo;
+
+Foo f;
+// expected-no-diagnostics
Index: clang/test/Modules/Inputs/inherit-attribute/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/module.modulemap
@@ -0,0 +1,3 @@
+module "b" { header "b.h" }
+
+module "c" { header "c.h" }
Index: clang/test/Modules/Inputs/inherit-attribute/c.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/c.h
@@ -0,0 +1,7 @@
+#include "a.h"
+
+class Foo;
+class C {
+public:
+  C();
+};
Index: clang/test/Modules/Inputs/inherit-attribute/b.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/b.h
@@ -0,0 +1,12 @@
+#include "a.h"
+
+class Foo;
+
+void bar() {
+  ::step;
+}
+
+class B {
+public:
+  B();
+};
Index: clang/test/Modules/Inputs/inherit-attribute/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/a.h
@@ -0,0 +1,10 @@
+#ifndef FOO
+#define FOO
+
+class Foo {
+public:
+  void step(int v);
+  Foo();
+};
+
+#endif
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -281,6 +281,9 @@
 static Decl *getMostRecentDeclImpl(...);
 static Decl *getMostRecentDecl(Decl *D);
 
+static void mergeInheritableAttributes(ASTReader , Decl *D,
+   Decl *Previous);
+
 template 
 static void attachPreviousDeclImpl(ASTReader ,
Redeclarable *D, Decl *Previous,
@@ -3531,6 +3534,19 @@
   return ASTDeclReader::getMostRecentDecl(D->getCanonicalDecl());
 }
 
+void ASTDeclReader::mergeInheritableAttributes(ASTReader , Decl *D,
+   Decl *Previous) {
+  InheritableAttr *NewAttr = nullptr;
+  ASTContext  = Reader.getContext();
+  const auto *IA = Previous->getAttr();
+
+  if (IA && !D->hasAttr()) {
+NewAttr = cast(IA->clone(Context));
+NewAttr->setInherited(true);
+D->addAttr(NewAttr);
+  }
+}
+
 template
 void ASTDeclReader::attachPreviousDeclImpl(ASTReader ,
Redeclarable *D,
@@ -3689,6 +3705,12 @@
   if (auto *TD = dyn_cast(D))
 inheritDefaultTemplateArguments(Reader.getContext(),
 cast(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-14 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3544
+const auto *IA =
+dyn_cast(Previous->getAttr());
+

aaron.ballman wrote:
> 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();
> if (IA && Previous->hasAttr()) {
>   ...
> }
> ```
I'm really sorry but I had a think-o in my suggestion. What I really meant was:
```
const auto *IA = Previous->getAttr();
if (IA && !D->hasAttr()) {
  ...
}
```
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?


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

https://reviews.llvm.org/D83174



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-10 Thread Vaibhav Garg via Phabricator via cfe-commits 
gargvaibhav64 updated this revision to Diff 277022.
gargvaibhav64 marked 3 inline comments as done.
gargvaibhav64 added a comment.

Incorporated the changes


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

https://reviews.llvm.org/D83174

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/inherit-attribute/a.h
  clang/test/Modules/Inputs/inherit-attribute/b.h
  clang/test/Modules/Inputs/inherit-attribute/c.h
  clang/test/Modules/Inputs/inherit-attribute/module.modulemap
  clang/test/Modules/inherit-attribute.cpp

Index: clang/test/Modules/inherit-attribute.cpp
===
--- /dev/null
+++ clang/test/Modules/inherit-attribute.cpp
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -ast-dump -I%S/Inputs/inherit-attribute -fmodules-cache-path=%t -fimplicit-module-maps -verify %s -fmodules-local-submodule-visibility
+
+#include "b.h"
+#include "c.h"
+
+class Foo;
+
+Foo f;
+// expected-no-diagnostics
Index: clang/test/Modules/Inputs/inherit-attribute/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/module.modulemap
@@ -0,0 +1,3 @@
+module "b" { header "b.h" }
+
+module "c" { header "c.h" }
Index: clang/test/Modules/Inputs/inherit-attribute/c.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/c.h
@@ -0,0 +1,7 @@
+#include "a.h"
+
+class Foo;
+class C {
+public:
+  C();
+};
Index: clang/test/Modules/Inputs/inherit-attribute/b.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/b.h
@@ -0,0 +1,12 @@
+#include "a.h"
+
+class Foo;
+
+void bar() {
+  ::step;
+}
+
+class B {
+public:
+  B();
+};
Index: clang/test/Modules/Inputs/inherit-attribute/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/a.h
@@ -0,0 +1,10 @@
+#ifndef FOO
+#define FOO
+
+class Foo {
+public:
+  void step(int v);
+  Foo();
+};
+
+#endif
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -281,6 +281,9 @@
 static Decl *getMostRecentDeclImpl(...);
 static Decl *getMostRecentDecl(Decl *D);
 
+static void mergeInheritableAttributes(ASTReader , Decl *D,
+   Decl *Previous);
+
 template 
 static void attachPreviousDeclImpl(ASTReader ,
Redeclarable *D, Decl *Previous,
@@ -3531,6 +3534,19 @@
   return ASTDeclReader::getMostRecentDecl(D->getCanonicalDecl());
 }
 
+void ASTDeclReader::mergeInheritableAttributes(ASTReader , Decl *D,
+   Decl *Previous) {
+  InheritableAttr *NewAttr = nullptr;
+  ASTContext  = Reader.getContext();
+  const auto *IA = Previous->getAttr();
+
+  if (IA && Previous->hasAttr()) {
+NewAttr = cast(IA->clone(Context));
+NewAttr->setInherited(true);
+D->addAttr(NewAttr);
+  }
+}
+
 template
 void ASTDeclReader::attachPreviousDeclImpl(ASTReader ,
Redeclarable *D,
@@ -3689,6 +3705,12 @@
   if (auto *TD = dyn_cast(D))
 inheritDefaultTemplateArguments(Reader.getContext(),
 cast(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-10 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

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(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?




Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3544
+const auto *IA =
+dyn_cast(Previous->getAttr());
+

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();
if (IA && Previous->hasAttr()) {
  ...
}
```



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3546
+
+NewAttr = new (Context) MSInheritanceAttr(
+IA->getRange(), Context, IA->getBestCase(), 
IA->getSpellingListIndex());

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



Comment at: clang/test/Modules/Inputs/inherit-attribute/a.h:11
+#endif
\ No newline at end of file


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


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

https://reviews.llvm.org/D83174



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-09 Thread Vaibhav Garg via Phabricator via cfe-commits 
gargvaibhav64 updated this revision to Diff 276680.
gargvaibhav64 marked 3 inline comments as done.
gargvaibhav64 added a comment.

Added a new attribute instead of using the previous one


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

https://reviews.llvm.org/D83174

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/inherit-attribute/a.h
  clang/test/Modules/Inputs/inherit-attribute/b.h
  clang/test/Modules/Inputs/inherit-attribute/c.h
  clang/test/Modules/Inputs/inherit-attribute/module.modulemap
  clang/test/Modules/inherit-attribute.cpp

Index: clang/test/Modules/inherit-attribute.cpp
===
--- /dev/null
+++ clang/test/Modules/inherit-attribute.cpp
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -ast-dump -I%S/Inputs/inherit-attribute -fmodules-cache-path=%t -fimplicit-module-maps -verify %s -fmodules-local-submodule-visibility
+
+#include "b.h"
+#include "c.h"
+
+class Foo;
+
+Foo f;
+// expected-no-diagnostics
\ No newline at end of file
Index: clang/test/Modules/Inputs/inherit-attribute/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/module.modulemap
@@ -0,0 +1,3 @@
+module "b" { header "b.h" }
+
+module "c" { header "c.h" }
\ No newline at end of file
Index: clang/test/Modules/Inputs/inherit-attribute/c.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/c.h
@@ -0,0 +1,7 @@
+#include "a.h"
+
+class Foo;
+class C {
+public:
+  C();
+};
\ No newline at end of file
Index: clang/test/Modules/Inputs/inherit-attribute/b.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/b.h
@@ -0,0 +1,12 @@
+#include "a.h"
+
+class Foo;
+
+void bar() {
+  ::step;
+}
+
+class B {
+public:
+  B();
+};
\ No newline at end of file
Index: clang/test/Modules/Inputs/inherit-attribute/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/a.h
@@ -0,0 +1,10 @@
+#ifndef FOO
+#define FOO
+
+class Foo {
+public:
+  void step(int v);
+  Foo();
+};
+
+#endif
\ No newline at end of file
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -281,6 +281,9 @@
 static Decl *getMostRecentDeclImpl(...);
 static Decl *getMostRecentDecl(Decl *D);
 
+static void mergeInheritableAttributes(ASTReader , Decl *D,
+   Decl *Previous);
+
 template 
 static void attachPreviousDeclImpl(ASTReader ,
Redeclarable *D, Decl *Previous,
@@ -3531,6 +3534,23 @@
   return ASTDeclReader::getMostRecentDecl(D->getCanonicalDecl());
 }
 
+void ASTDeclReader::mergeInheritableAttributes(ASTReader , Decl *D,
+   Decl *Previous) {
+  InheritableAttr *NewAttr = nullptr;
+  ASTContext  = Reader.getContext();
+  if (Previous->hasAttr() &&
+  !D->hasAttr()) {
+const auto *IA =
+dyn_cast(Previous->getAttr());
+
+NewAttr = new (Context) MSInheritanceAttr(
+IA->getRange(), Context, IA->getBestCase(), IA->getSpellingListIndex());
+
+NewAttr->setInherited(true);
+D->addAttr(NewAttr);
+  }
+}
+
 template
 void ASTDeclReader::attachPreviousDeclImpl(ASTReader ,
Redeclarable *D,
@@ -3689,6 +3709,12 @@
   if (auto *TD = dyn_cast(D))
 inheritDefaultTemplateArguments(Reader.getContext(),
 cast(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-08 Thread Vaibhav Garg via Phabricator via cfe-commits 
gargvaibhav64 updated this revision to Diff 276351.
gargvaibhav64 edited the summary of this revision.
gargvaibhav64 added a comment.

I incorporated the changes


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

https://reviews.llvm.org/D83174

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/inherit-attribute/a.h
  clang/test/Modules/Inputs/inherit-attribute/b.h
  clang/test/Modules/Inputs/inherit-attribute/c.h
  clang/test/Modules/Inputs/inherit-attribute/module.modulemap
  clang/test/Modules/inherit-attribute.cpp

Index: clang/test/Modules/inherit-attribute.cpp
===
--- /dev/null
+++ clang/test/Modules/inherit-attribute.cpp
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -ast-dump -I%S/Inputs/inherit-attribute -fmodules-cache-path=%t -fimplicit-module-maps -verify %s -fmodules-local-submodule-visibility
+
+#include "b.h"
+#include "c.h"
+
+class Foo;
+
+Foo f;
+// expected-no-diagnostics
\ No newline at end of file
Index: clang/test/Modules/Inputs/inherit-attribute/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/module.modulemap
@@ -0,0 +1,3 @@
+module "b" { header "b.h" }
+
+module "c" { header "c.h" }
\ No newline at end of file
Index: clang/test/Modules/Inputs/inherit-attribute/c.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/c.h
@@ -0,0 +1,7 @@
+#include "a.h"
+
+class Foo;
+class C {
+public:
+  C();
+};
\ No newline at end of file
Index: clang/test/Modules/Inputs/inherit-attribute/b.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/b.h
@@ -0,0 +1,12 @@
+#include "a.h"
+
+class Foo;
+
+void bar() {
+  ::step;
+}
+
+class B {
+public:
+  B();
+};
\ No newline at end of file
Index: clang/test/Modules/Inputs/inherit-attribute/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/a.h
@@ -0,0 +1,10 @@
+#ifndef FOO
+#define FOO
+
+class Foo {
+public:
+  void step(int v);
+  Foo();
+};
+
+#endif
\ No newline at end of file
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -281,6 +281,8 @@
 static Decl *getMostRecentDeclImpl(...);
 static Decl *getMostRecentDecl(Decl *D);
 
+static void mergeInheritableAttributes(Decl *D, Decl *Previous);
+
 template 
 static void attachPreviousDeclImpl(ASTReader ,
Redeclarable *D, Decl *Previous,
@@ -3531,6 +3533,17 @@
   return ASTDeclReader::getMostRecentDecl(D->getCanonicalDecl());
 }
 
+void ASTDeclReader::mergeInheritableAttributes(Decl *D, Decl *Previous) {
+  InheritableAttr *IA = nullptr;
+  if (Previous->hasAttr() &&
+  !D->hasAttr()) {
+IA = cast(
+(Previous->getAttr())->clone(D->getASTContext()));
+IA->setInherited(true);
+D->addAttr(IA);
+  }
+}
+
 template
 void ASTDeclReader::attachPreviousDeclImpl(ASTReader ,
Redeclarable *D,
@@ -3689,6 +3702,12 @@
   if (auto *TD = dyn_cast(D))
 inheritDefaultTemplateArguments(Reader.getContext(),
 cast(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(D, Previous);
 }
 
 template
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-07 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

@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?




Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3540
+Attr *IA = Previous->getAttr();
+D->addAttr(IA);
+  } else if (!Previous->hasAttr() &&

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.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3544
+Attr *IA = D->getAttr();
+Previous->addAttr(IA);
+  }

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



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3709
+  // it needs to be added to all the declarations in the redeclarable chain.
+  mergeInheritableAttributes(D, Previous);
 }

Please add a FIXME to do this is general.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83174



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

2020-07-05 Thread Vaibhav Garg via Phabricator via cfe-commits 
gargvaibhav64 created this revision.
gargvaibhav64 added reviewers: rsmith, v.g.vassilev.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This commit attaches teaches ASTDeclReader::attachPreviousDecl to successfully 
merge two Decl's when one contains an inheritable attribute like the 
MSInheritanceAttr. Usually, attributes that are needed to 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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83174

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/inherit-attribute/a.h
  clang/test/Modules/Inputs/inherit-attribute/b.h
  clang/test/Modules/Inputs/inherit-attribute/c.h
  clang/test/Modules/Inputs/inherit-attribute/module.modulemap
  clang/test/Modules/inherit-attribute.cpp

Index: clang/test/Modules/inherit-attribute.cpp
===
--- /dev/null
+++ clang/test/Modules/inherit-attribute.cpp
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -ast-dump -I%S/Inputs/inherit-attribute -fmodules-cache-path=%t -fimplicit-module-maps -verify %s -fmodules-local-submodule-visibility
+
+#include "b.h"
+#include "c.h"
+
+class Foo;
+
+Foo f;
+// expected-no-diagnostics
\ No newline at end of file
Index: clang/test/Modules/Inputs/inherit-attribute/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/module.modulemap
@@ -0,0 +1,3 @@
+module "b" { header "b.h" }
+
+module "c" { header "c.h" }
\ No newline at end of file
Index: clang/test/Modules/Inputs/inherit-attribute/c.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/c.h
@@ -0,0 +1,7 @@
+#include "a.h"
+
+class Foo;
+class C {
+public:
+  C();
+};
\ No newline at end of file
Index: clang/test/Modules/Inputs/inherit-attribute/b.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/b.h
@@ -0,0 +1,12 @@
+#include "a.h"
+
+class Foo;
+
+void bar() {
+  ::step;
+}
+
+class B {
+public:
+  B();
+};
\ No newline at end of file
Index: clang/test/Modules/Inputs/inherit-attribute/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/inherit-attribute/a.h
@@ -0,0 +1,10 @@
+#ifndef FOO
+#define FOO
+
+class Foo {
+public:
+  void step(int v);
+  Foo();
+};
+
+#endif
\ No newline at end of file
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -281,6 +281,8 @@
 static Decl *getMostRecentDeclImpl(...);
 static Decl *getMostRecentDecl(Decl *D);
 
+static void mergeInheritableAttributes(Decl *D, Decl *Previous);
+
 template 
 static void attachPreviousDeclImpl(ASTReader ,
Redeclarable *D, Decl *Previous,
@@ -3531,6 +3533,18 @@
   return ASTDeclReader::getMostRecentDecl(D->getCanonicalDecl());
 }
 
+void ASTDeclReader::mergeInheritableAttributes(Decl *D, Decl *Previous) {
+  if (Previous->hasAttr() &&
+  !D->hasAttr()) {
+Attr *IA = Previous->getAttr();
+D->addAttr(IA);
+  } else if (!Previous->hasAttr() &&
+ D->hasAttr()) {
+Attr *IA = D->getAttr();
+Previous->addAttr(IA);
+  }
+}
+
 template
 void ASTDeclReader::attachPreviousDeclImpl(ASTReader ,
Redeclarable *D,
@@ -3689,6 +3703,10 @@
   if (auto *TD = dyn_cast(D))
 inheritDefaultTemplateArguments(Reader.getContext(),
 cast(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.
+  mergeInheritableAttributes(D, Previous);
 }
 
 template
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits