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 not test this specific corner case, I've managed to manually narrow it down
and write a reliable regression test. The core issue boils down to having a
non-trivially destructible member in a superclass that lacks a destructor and a
subclass that lacks a destructor, in which case, a different path was taken to
balance out the static storage duration class' initializer call and the
`__cxa_atexit` registration. This adds an explicit check for the attribute and
avoids balancing out the constructor as intended by the attribute.
The new test successfully replicates the assertion failure and should fail for
the above cases in assertion builds. Without assertions the generated code
produces undefined behavior if the above conditions are met. There is a
separate test for this attribute that provides the coverage for its
functionality, this is a much more narrower test for a very specific scenario
in which it was possible to cause an improperly balanced constructor call
followed by a emission of a call to a destructor that would never be emitted
due to the attribute, thus tripping the assert. Now no attempt to call the
destructor is made if the declaration is marked as `no_destroy`.
`Test without patch:`
=> ninja check-clang-semacxx
FAIL: Clang :: SemaCXX/attr-no-destroy-d54344.cpp (164 of 818)
******************** TEST 'Clang :: SemaCXX/attr-no-destroy-d54344.cpp'
FAILED ********************
(--- stack trace and the expected assertion failure ---)
/q/org.llvm.caches/llvm-8.0/4141/tools/clang/test/SemaCXX/Output/attr-no-destroy-d54344.cpp.script:
line 1: 31356 Aborted (core dumped)
--
********************
Testing Time: 5.03s
********************
Failing Tests (1):
Clang :: SemaCXX/attr-no-destroy-d54344.cpp
Expected Passes : 816
Expected Failures : 1
Unexpected Failures: 1
FAILED: tools/clang/test/CMakeFiles/check-clang-semacxx
ninja: build stopped: subcommand failed.
`Patch applied:`
=> ninja check-clang-semacxx
Testing Time: 6.28s
Expected Passes : 817
Expected Failures : 1
Repository:
rC Clang
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
===================================================================
--- test/SemaCXX/attr-no-destroy-d54344.cpp
+++ test/SemaCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,92 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -S -O0 %s -o - > /dev/null
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+namespace util {
+ template <typename T>
+ class test_vector
+ {
+ public:
+ test_vector(unsigned c)
+ : Used(0), TotalSize(sizeof(T) * c)
+ {
+ // Intentional
+ Storage = (T*)(new T[TotalSize]);
+ }
+ ~test_vector() {
+ delete[] Storage;
+ }
+ char* data() {
+ return Storage;
+ }
+ unsigned size() {
+ return TotalSize;
+ }
+ void push_back(T one_thing) {
+ Storage[Used++] = one_thing;
+ }
+ private:
+ unsigned Used;
+ unsigned TotalSize;
+ T* Storage;
+ };
+}
+
+volatile int do_not_optimize_me = 0;
+
+namespace os {
+ using char_vec_t = util::test_vector<char>;
+
+ class logger_base
+ {
+ public:
+ constexpr logger_base() = default;
+ protected:
+ char_vec_t* NamePtr = nullptr;
+ char_vec_t Name = char_vec_t(10);
+ };
+
+ class logger : public logger_base
+ {
+ public:
+ void stuff(const char* fmt, int something) {
+ do_not_optimize_me = something + fmt[0];
+ }
+ logger()
+ {
+ Name = char_vec_t(8);
+ Name.push_back('a');
+ }
+
+ logger(const char* name)
+ {
+ Name = util::test_vector<char>(__builtin_strlen(name));
+ Name.push_back(name[0]);
+ Name.push_back(name[1]);
+ }
+ };
+}
+
+#define TOPLEVEL_LOGGER(_var_name, _category_const) \
+ namespace { constexpr char $__##_var_name[] = _category_const; \
+ __attribute((no_destroy)) os::logger _var_name ($__##_var_name) ; }
+
+TOPLEVEL_LOGGER(s_log, "something");
+
+class some_class {
+public:
+ some_class(int some_value) {
+ do_not_optimize_me = some_value;
+ s_log.stuff("wawawa", 5 + do_not_optimize_me);
+ }
+ ~some_class() = default;
+};
+
+static some_class s_ctor(1);
\ No newline at end of file
Index: lib/CodeGen/CGDeclCXX.cpp
===================================================================
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -64,6 +64,14 @@
/// static storage duration.
static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
ConstantAddress Addr) {
+ // Honor __attribute__((no_destroy)) and bail instead of attempting
+ // to emit a reference to a possibly non-existant destructor, which
+ // in turn can cause a crash. This will result in a global constructor
+ // that isn't balanced out by a destructor call as intended by the
+ // attribute (D54344).
+ if (D.hasAttr<NoDestroyAttr>())
+ return;
+
CodeGenModule &CGM = CGF.CGM;
// FIXME: __attribute__((cleanup)) ?
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
