This revision was automatically updated to reflect the committed changes.
Closed by commit rL346628: [CodeGen][CXX]: Fix no_destroy CG bug under specific
circumstances (authored by kristina, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
erik.pilkington added a comment.
In https://reviews.llvm.org/D54344#1294960, @kristina wrote:
> In https://reviews.llvm.org/D54344#1294942, @erik.pilkington wrote:
>
> > LGTM too, with one small fix in test. Thanks for working on this!
>
>
> Well, I figured since the assertion being tripped was
kristina added a comment.
Though on the other hand it makes no sense to skip it with asserts off either,
that just clicked in my head, sorry.
Repository:
rC Clang
https://reviews.llvm.org/D54344
___
cfe-commits mailing list
kristina updated this revision to Diff 173602.
kristina added a comment.
Revised to address comments.
Repository:
rC Clang
https://reviews.llvm.org/D54344
Files:
lib/CodeGen/CGDeclCXX.cpp
test/CodeGenCXX/attr-no-destroy-d54344.cpp
Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
kristina added a comment.
In https://reviews.llvm.org/D54344#1294942, @erik.pilkington wrote:
> LGTM too, with one small fix in test. Thanks for working on this!
Well, I figured since the assertion being tripped was (before investigation)
the only reliable way to notice the bug it makes sense
erik.pilkington accepted this revision.
erik.pilkington added a comment.
LGTM too, with one small fix in test. Thanks for working on this!
Comment at: test/CodeGenCXX/attr-no-destroy-d54344.cpp:4
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu
kristina updated this revision to Diff 173534.
kristina added a comment.
Revised to address nitpicks.
Repository:
rC Clang
https://reviews.llvm.org/D54344
Files:
lib/CodeGen/CGDeclCXX.cpp
test/CodeGenCXX/attr-no-destroy-d54344.cpp
Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM aside from some small commenting nits. However, I leave it to
@erik.pilkington to make the final sign-off.
Comment at: lib/CodeGen/CGDeclCXX.cpp:68
+ //
kristina updated this revision to Diff 173530.
kristina set the repository for this revision to rC Clang.
kristina added a comment.
@erik.pilkington Fantastic catch, I totally forgot about the global flag.
Revised, moved to CodeGenCXX, added a test to verify ctor/dtor is balanced
under normal
erik.pilkington added a comment.
I have a few nits, but I think this looks about right.
I reduced this testcase with creduce, can you use the minimized version? Also,
I think it makes more sense to put the test into `test/CodeGenCXX` and verify
the `-emit-llvm-only` output is correct with
kristina updated this revision to Diff 173507.
kristina added a comment.
Revised, added a newline to the testcase.
https://reviews.llvm.org/D54344
Files:
lib/CodeGen/CGDeclCXX.cpp
test/SemaCXX/attr-no-destroy-d54344.cpp
Index: test/SemaCXX/attr-no-destroy-d54344.cpp
lebedev.ri added inline comments.
Comment at: test/SemaCXX/attr-no-destroy-d54344.cpp:93
+static some_class s_ctor(1);
\ No newline at end of file
Please add new line.
Repository:
rC Clang
https://reviews.llvm.org/D54344
kristina updated this revision to Diff 173503.
kristina set the repository for this revision to rC Clang.
kristina added a comment.
Revised, I think I worked out what was happening, there's an explanation in the
comment in the test. This is a relatively new attribute so the coverage for it
did
kristina added a comment.
I've managed to isolate enough to make for a testcase. Is that too broad or is
it okay to attach as is?
The following triggers the assert:
namespace util {
template
class test_vector
{
public:
test_vector(unsigned c)
kristina added a comment.
This seems to be a peculiar side effect of weird combinations of previous uses
of attributes `no_destroy`, `used`, and `section`, as well as complex macros
used to create metaclass-like systems (through the use of said attributes). It
seems that there is a serious bug
kristina added a comment.
In https://reviews.llvm.org/D54344#1293643, @aaron.ballman wrote:
> In https://reviews.llvm.org/D54344#1293468, @erik.pilkington wrote:
>
> > Have you tried running creduce on the preprocessed source? We should really
> > have a real reproducer for this, otherwise its
aaron.ballman added a comment.
In https://reviews.llvm.org/D54344#1293468, @erik.pilkington wrote:
> Have you tried running creduce on the preprocessed source? We should really
> have a real reproducer for this, otherwise its really hard to tell what the
> underlying problem is.
I'm going to
kristina added a comment.
In https://reviews.llvm.org/D54344#1293468, @erik.pilkington wrote:
> Have you tried running creduce on the preprocessed source? We should really
> have a real reproducer for this, otherwise its really hard to tell what the
> underlying problem is.
Unable to build
erik.pilkington added a reviewer: erik.pilkington.
erik.pilkington added a comment.
Have you tried running creduce on the preprocessed source? We should really
have a real reproducer for this, otherwise its really hard to tell what the
underlying problem is.
https://reviews.llvm.org/D54344
kristina updated this revision to Diff 173411.
kristina added a comment.
Revised (style/ordering).
https://reviews.llvm.org/D54344
Files:
lib/CodeGen/CGDeclCXX.cpp
Index: lib/CodeGen/CGDeclCXX.cpp
===
---
kristina created this revision.
kristina added reviewers: rsmith, clang, JonasToth, EricWF, aaron.ballman.
Herald added subscribers: dexonsmith, mehdi_amini.
Herald added a reviewer: jfb.
Difficult to reproduce crash, attempted it in the test, it appears to not
trigger it. I can consistently
21 matches
Mail list logo
