Bug 70932 - flexible array member with non-trivial destructor
Summary: flexible array member with non-trivial destructor
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 6.1.0
: P3 minor
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks: flexmembers
  Show dependency treegraph
 
Reported: 2016-05-03 21:43 UTC by Jens Maurer
Modified: 2017-11-18 17:14 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.9.3, 5.3.0
Known to fail: 6.1.0, 7.2.0
Last reconfirmed: 2016-05-04 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jens Maurer 2016-05-03 21:43:11 UTC
Testcase:

struct A {
  ~A();
};

struct S {
  S() : i(0) { }
  int i;
  A a[];
};


$ g++ -v flex.cc
gcc version 6.1.0 (GCC) 

flex.cc: In constructor ‘S::S()’:
flex.cc:6:12: error: unknown array size in delete


This used to work with gcc 5.x.  (I understand the destructor of A is not called for a[] here, because the number of elements is unknown.)
Comment 1 Martin Sebor 2016-05-04 14:53:46 UTC
The problem was introduced r233126 as a result of switching the internal flexible array representation to use a null type domain.  Prior to 6.1, GCC treated flexible array members the same as arrays of zero elements.

I'll have to think about what the right solution is: reject flexible array members with non-trivial destructors with a better error (like clang does) or allow them with a warning.  My concern with allowing them is that while G++ 6.1 allows them to be initialized (5.x rejects the initialization which seems like a bug), it won't be able to invoke their destructor in code like this:

  struct A {
    int i;
    ~A() { __builtin_printf ("%s\n", __PRETTY_FUNCTION__); }
  };

  struct S {
    int i;
    A a[];
  };

  int main ()
  {
    S s = { 3, { 1, 2, 3 } };
  }
Comment 2 Jens Maurer 2016-05-04 17:26:41 UTC
The whole point of flexible array members seems to be to save an allocation for the array, with the precondition that the array size can be determined at initialization time and stays fixed for the entire lifetime of the enclosing struct.  In that scenario, I need to placement-new the array elements (or, probably, the entire array) anyway, so requiring to explicitly call the destructor(s) seems a natural duality.

In the example
    S s = { 3, { 1, 2, 3 } };
it seems surprising that the destructor for the elements is not called, given that the number of elements is "right there" in the source code. [Yes, I have an idea about the implementation difficulties here.] I'd rather prefer this initialization example to be ill-formed, if the element type has a non-trivial destructor.  Consider if the element type is a std::string (with dynamic allocation) and this is inside a loop; this seems to cause a serious memory leak.
Comment 3 Paul Wankadia 2016-07-21 12:13:24 UTC
https://github.com/google/re2/issues/102 reported the same error with GCC 6.1.1:


g++ -c -o obj/dbg/re2/dfa.o  -std=c++11 -pthread -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -I.   -O3 -g re2/dfa.cc
re2/dfa.cc: In Konstruktor »re2::DFA::State::State()«:
re2/dfa.cc:95:10: Fehler: unbekannte Feldgröße in »delete«
   struct State {
          ^~~~~
re2/dfa.cc: In Elementfunktion »re2::DFA::State* re2::DFA::CachedState(int*, int, re2::uint)«:
re2/dfa.cc:703:9: Anmerkung: erzeugte Methode »re2::DFA::State::State()« zuerst hier erfordert 
   State state;
         ^~~~~
make: *** [Makefile:190: obj/dbg/re2/dfa.o] Fehler 1


This is the flexible array member:

    std::atomic<State*> next_[];

Is this likely to be the same issue even though std::atomic<T*> should have a trivial destructor for all T?
Comment 4 Paul Wankadia 2016-07-21 12:19:39 UTC
FWIW, using a simplified struct, Compiler Explorer (gcc.godbolt.org) with GCC 6.1 throws a different error:


#include <atomic>

struct State {
  int i;
  std::atomic<State*> next_[];
};


Compiler output — x86 gcc 6.1 (g++ (DRW-internal-build) 6.1.0)
5 : error: field 'next_' has incomplete type 'std::atomic<State*> []'
std::atomic<State*> next_[];
^
In file included from /tmp/gcc-explorer-compiler116621-82-1gzc8bu/example.cpp:1:0:
/opt/gcc-6.1.0/include/c++/6.1.0/atomic:165:12: note: declaration of 'struct std::atomic<State*>'
Comment 5 Martin Sebor 2016-07-21 14:32:41 UTC
(In reply to Paul Wankadia from comment #3)
> Is this likely to be the same issue even though std::atomic<T*> should have
> a trivial destructor for all T?

No, that's bug 71147, fixed in 7.0 and 6.x.
Comment 6 Martin Sebor 2016-07-21 15:32:38 UTC
To clarify/correct comment #5: the error in comment #4 is due to bug 71147, the one in comment #3 ("unbekannte Feldgröße in »delete«") looks like it's the same as in comment #0 and probably due to this bug.
Comment 7 Paul Wankadia 2016-07-21 15:54:47 UTC
Ahh. Thank you for clarifying. I will continue to watch this bug then. :)
Comment 8 Carlos Tripiana Montes 2016-09-29 11:18:16 UTC
(In reply to Paul Wankadia from comment #7)
> Ahh. Thank you for clarifying. I will continue to watch this bug then. :)

Any updates on this??
Comment 9 Martin Sebor 2016-09-29 14:32:49 UTC
Sorry, I haven't gotten to it yet.
Comment 10 Paul Wankadia 2017-11-18 03:14:42 UTC
(In reply to Martin Sebor from comment #9)
> Sorry, I haven't gotten to it yet.

Was this bug fixed in GCC 7.x? :)

I ask because the workaround in RE2 is conditioned thus:

#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ == 6 && __GNUC_MINOR__ >= 1

No one has reported breakage with GCC 7.x (which is why I had forgotten about the bug until just now!) and our build matrix on Travis CI is clean.
Comment 11 Martin Sebor 2017-11-18 17:14:20 UTC
(In reply to Paul Wankadia from comment #10)

There has been no change since the last comment and I'm not currently working on it.  Since Clang rejects the test case as well I'm also not too inclined to change GCC to accept the code.  (I do note that Intel ICC accepts it, possibly for compatibility with GCC 5.)

To make this work I would suggest to use a zero-length array instead.  That makes it clear that a GCC-specific extension is being used and it's also accepted by Clang and ICC.