On 6/29/21 8:43 AM, Jason Merrill wrote:
On 6/28/21 2:07 PM, Martin Sebor wrote:
On 6/28/21 2:07 AM, Richard Biener wrote:
On Sat, Jun 26, 2021 at 12:36 AM Martin Sebor <mse...@gmail.com> wrote:
On 6/25/21 4:11 PM, Jason Merrill wrote:
On 6/25/21 4:51 PM, Martin Sebor wrote:
On 6/1/21 3:38 PM, Jason Merrill wrote:
On 6/1/21 3:56 PM, Martin Sebor wrote:
On 5/27/21 2:53 PM, Jason Merrill wrote:
On 4/27/21 11:52 AM, Martin Sebor via Gcc-patches wrote:
On 4/27/21 8:04 AM, Richard Biener wrote:
On Tue, Apr 27, 2021 at 3:59 PM Martin Sebor <mse...@gmail.com>
wrote:
On 4/27/21 1:58 AM, Richard Biener wrote:
On Tue, Apr 27, 2021 at 2:46 AM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
PR 90904 notes that auto_vec is unsafe to copy and assign
because
the class manages its own memory but doesn't define (or
delete)
either special function. Since I first ran into the problem,
auto_vec has grown a move ctor and move assignment from
a dynamically-allocated vec but still no copy ctor or copy
assignment operator.
The attached patch adds the two special functions to auto_vec
along
with a few simple tests. It makes auto_vec safe to use in
containers
that expect copyable and assignable element types and passes
bootstrap
and regression testing on x86_64-linux.
The question is whether we want such uses to appear since
those
can be quite inefficient? Thus the option is to delete those
operators?
I would strongly prefer the generic vector class to have the
properties
expected of any other generic container: copyable and
assignable. If
we also want another vector type with this restriction I
suggest
to add
another "noncopyable" type and make that property explicit in
its name.
I can submit one in a followup patch if you think we need one.
I'm not sure (and not strictly against the copy and assign).
Looking around
I see that vec<> does not do deep copying. Making auto_vec<>
do it
might be surprising (I added the move capability to match how
vec<>
is used - as "reference" to a vector)
The vec base classes are special: they have no ctors at all
(because
of their use in unions). That's something we might have to
live with
but it's not a model to follow in ordinary containers.
I don't think we have to live with it anymore, now that we're
writing C++11.
The auto_vec class was introduced to fill the need for a
conventional
sequence container with a ctor and dtor. The missing copy
ctor and
assignment operators were an oversight, not a deliberate feature.
This change fixes that oversight.
The revised patch also adds a copy ctor/assignment to the
auto_vec
primary template (that's also missing it). In addition, it adds
a new class called auto_vec_ncopy that disables copying and
assignment as you prefer.
Hmm, adding another class doesn't really help with the confusion
richi mentions. And many uses of auto_vec will pass them as vec,
which will still do a shallow copy. I think it's probably better
to disable the copy special members for auto_vec until we fix
vec<>.
There are at least a couple of problems that get in the way of
fixing
all of vec to act like a well-behaved C++ container:
1) The embedded vec has a trailing "flexible" array member with its
instances having different size. They're initialized by memset and
copied by memcpy. The class can't have copy ctors or assignments
but it should disable/delete them instead.
2) The heap-based vec is used throughout GCC with the assumption of
shallow copy semantics (not just as function arguments but also as
members of other such POD classes). This can be changed by
providing
copy and move ctors and assignment operators for it, and also for
some of the classes in which it's a member and that are used with
the same assumption.
3) The heap-based vec::block_remove() assumes its elements are
PODs.
That breaks in VEC_ORDERED_REMOVE_IF (used in gcc/dwarf2cfi.c:2862
and tree-vect-patterns.c).
I took a stab at both and while (1) is easy, (2) is shaping up to
be a big and tricky project. Tricky because it involves using
std::move in places where what's moved is subsequently still used.
I can keep plugging away at it but it won't change the fact that
the embedded and heap-based vecs have different requirements.
It doesn't seem to me that having a safely copyable auto_vec needs
to be put on hold until the rats nest above is untangled. It won't
make anything worse than it is. (I have a project that depends on
a sane auto_vec working).
A couple of alternatives to solving this are to use std::vector or
write an equivalent vector class just for GCC.
It occurs to me that another way to work around the issue of passing
an auto_vec by value as a vec, and thus doing a shallow copy, would
be to add a vec ctor taking an auto_vec, and delete that. This
would
mean if you want to pass an auto_vec to a vec interface, it needs to
be by reference. We might as well do the same for operator=, though
that isn't as important.
Thanks, that sounds like a good idea. Attached is an implementation
of this change. Since the auto_vec copy ctor and assignment have
been deleted by someone else in the interim, this patch doesn't
reverse that. I will propose it separately after these changes
are finalized.
My approach was to 1) disable the auto_vec to vec conversion,
2) introduce an auto_vec::to_vec() to make the conversion possible
explicitly, and 3) resolve compilation errors by either changing
APIs to take a vec by reference or callers to convert auto_vec to
vec explicitly by to_vec(). In (3) I tried to minimize churn while
improving the const-correctness of the APIs.
What did you base the choice between reference or to_vec on? For
instance, it seems like c_parser_declaration_or_fndef could use a
reference, but you changed the callers instead.
I went with a reference whenever I could. That doesn't work when
there are callers that pass in a vNULL, so there, and in assignments,
I used to_vec().
Is there a way to "fix" the ugliness with vNULL? All those functions
should be able to use const vec<>& as otherwise they'd leak memory?
Can't we pass vNULL to a const vec<>&?
vNULL can bind to a const vec& (via the vec conversion ctor) but
not to vec&. The three functions that in the patch are passed
vNULL modify the argument when it's not vNULL but not otherwise.
The c_parser_declaration_or_fndef case is rather ugly: the vec is passed
by value, but then the modifications in c_finish_omp_declare_simd modify
the original vec.
We could keep the same semantic problem and make it more blatant by
changing to const vec& and doing a const_cast in
c_finish_omp_declare_simd before modifying the vec.
Do the other two have the same problem?
Yes, the functions that take a vec by value and are passed an auto_vec
"by reference" (the result of to_vec()) modify the auto_vec. This is
the "bug" this patch is designed to keep from happening by accident,
while letting the API clients do it intentionally.
Changing these APIs to take a const vec& while still letting them
modify the argument by casting away the constness seems even more
surprising to me than the current by-value style.
I do think it should be fixed but I'd have been more comfortable
handling that separately. Attached is a (near) minimal change
along these lines to c_parser_declaration_or_fndef and its callers.
The logic isn't exactly the same as the original but no tests fail.
If this is the direction we want to go in I can see about making
an analogous change to the other two similar functions in the patch.
Let me know.
Martin
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 27034f88f49..b77e5b4f5c0 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -1489,7 +1489,8 @@ static tree c_parser_std_attribute_specifier_sequence (c_parser *);
static void c_parser_external_declaration (c_parser *);
static void c_parser_asm_definition (c_parser *);
static void c_parser_declaration_or_fndef (c_parser *, bool, bool, bool,
- bool, bool, tree *, vec<c_token>,
+ bool, bool, tree * = NULL,
+ vec<c_token> * = NULL,
bool have_attrs = false,
tree attrs = NULL,
struct oacc_routine_data * = NULL,
@@ -1774,13 +1775,12 @@ c_parser_external_declaration (c_parser *parser)
an @interface or @protocol with prefix attributes). We can
only tell which after parsing the declaration specifiers, if
any, and the first declarator. */
- c_parser_declaration_or_fndef (parser, true, true, true, false, true,
- NULL, vNULL);
+ c_parser_declaration_or_fndef (parser, true, true, true, false, true);
break;
}
}
-static void c_finish_omp_declare_simd (c_parser *, tree, tree, vec<c_token>);
+static void c_finish_omp_declare_simd (c_parser *, tree, tree, vec<c_token> &);
static void c_finish_oacc_routine (struct oacc_routine_data *, tree, bool);
/* Build and add a DEBUG_BEGIN_STMT statement with location LOC. */
@@ -1890,11 +1890,15 @@ static void
c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
bool static_assert_ok, bool empty_ok,
bool nested, bool start_attr_ok,
- tree *objc_foreach_object_declaration,
- vec<c_token> omp_declare_simd_clauses,
- bool have_attrs, tree attrs,
- struct oacc_routine_data *oacc_routine_data,
- bool *fallthru_attr_p)
+ tree *objc_foreach_object_declaration
+ /* = NULL */,
+ vec<c_token> *omp_declare_simd_clauses
+ /* = NULL */,
+ bool have_attrs /* = false */,
+ tree attrs /* = NULL_TREE */,
+ struct oacc_routine_data *oacc_routine_data
+ /* = NULL */,
+ bool *fallthru_attr_p /* = NULL */)
{
struct c_declspecs *specs;
tree prefix_attrs;
@@ -2150,9 +2154,9 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
C_DTR_NORMAL, &dummy);
if (declarator == NULL)
{
- if (omp_declare_simd_clauses.exists ())
+ if (omp_declare_simd_clauses)
c_finish_omp_declare_simd (parser, NULL_TREE, NULL_TREE,
- omp_declare_simd_clauses);
+ *omp_declare_simd_clauses);
if (oacc_routine_data)
c_finish_oacc_routine (oacc_routine_data, NULL_TREE, false);
c_parser_skip_to_end_of_block_or_statement (parser);
@@ -2250,9 +2254,9 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
chainon (postfix_attrs, all_prefix_attrs));
if (!d)
d = error_mark_node;
- if (omp_declare_simd_clauses.exists ())
+ if (omp_declare_simd_clauses)
c_finish_omp_declare_simd (parser, d, NULL_TREE,
- omp_declare_simd_clauses);
+ *omp_declare_simd_clauses);
}
else
{
@@ -2262,9 +2266,9 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
chainon (postfix_attrs, all_prefix_attrs));
if (!d)
d = error_mark_node;
- if (omp_declare_simd_clauses.exists ())
+ if (omp_declare_simd_clauses)
c_finish_omp_declare_simd (parser, d, NULL_TREE,
- omp_declare_simd_clauses);
+ *omp_declare_simd_clauses);
init_loc = c_parser_peek_token (parser)->location;
rich_location richloc (line_table, init_loc);
start_init (d, asm_name, global_bindings_p (), &richloc);
@@ -2342,7 +2346,7 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
warn_parm_array_mismatch (lastloc, d, parms);
}
}
- if (omp_declare_simd_clauses.exists ())
+ if (omp_declare_simd_clauses)
{
tree parms = NULL_TREE;
if (d && TREE_CODE (d) == FUNCTION_DECL)
@@ -2360,7 +2364,7 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
if (parms)
temp_store_parm_decls (d, parms);
c_finish_omp_declare_simd (parser, d, parms,
- omp_declare_simd_clauses);
+ *omp_declare_simd_clauses);
if (parms)
temp_pop_parm_decls ();
}
@@ -2496,11 +2500,11 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
while (c_parser_next_token_is_not (parser, CPP_EOF)
&& c_parser_next_token_is_not (parser, CPP_OPEN_BRACE))
c_parser_declaration_or_fndef (parser, false, false, false,
- true, false, NULL, vNULL);
+ true, false);
store_parm_decls ();
- if (omp_declare_simd_clauses.exists ())
+ if (omp_declare_simd_clauses)
c_finish_omp_declare_simd (parser, current_function_decl, NULL_TREE,
- omp_declare_simd_clauses);
+ *omp_declare_simd_clauses);
if (oacc_routine_data)
c_finish_oacc_routine (oacc_routine_data, current_function_decl, true);
location_t startloc = c_parser_peek_token (parser)->location;
@@ -5699,7 +5703,7 @@ c_parser_compound_statement_nostart (c_parser *parser)
bool fallthru_attr_p = false;
c_parser_declaration_or_fndef (parser, true, !have_std_attrs,
true, true, true, NULL,
- vNULL, have_std_attrs, std_attrs,
+ NULL, have_std_attrs, std_attrs,
NULL, &fallthru_attr_p);
if (last_stmt && !fallthru_attr_p)
@@ -5731,7 +5735,7 @@ c_parser_compound_statement_nostart (c_parser *parser)
last_label = false;
mark_valid_location_for_stdc_pragma (false);
c_parser_declaration_or_fndef (parser, true, true, true, true,
- true, NULL, vNULL);
+ true);
/* Following the old parser, __extension__ does not
disable this diagnostic. */
restore_extension_diagnostics (ext);
@@ -6782,7 +6786,7 @@ c_parser_for_statement (c_parser *parser, bool ivdep, unsigned short unroll,
|| c_parser_nth_token_starts_std_attributes (parser, 1))
{
c_parser_declaration_or_fndef (parser, true, true, true, true, true,
- &object_expression, vNULL);
+ &object_expression);
parser->objc_could_be_foreach_context = false;
if (c_parser_next_token_is_keyword (parser, RID_IN))
@@ -6813,7 +6817,7 @@ c_parser_for_statement (c_parser *parser, bool ivdep, unsigned short unroll,
ext = disable_extension_diagnostics ();
c_parser_consume_token (parser);
c_parser_declaration_or_fndef (parser, true, true, true, true,
- true, &object_expression, vNULL);
+ true, &object_expression);
parser->objc_could_be_foreach_context = false;
restore_extension_diagnostics (ext);
@@ -11277,7 +11281,7 @@ c_parser_objc_methodprotolist (c_parser *parser)
}
else
c_parser_declaration_or_fndef (parser, false, false, true,
- false, true, NULL, vNULL);
+ false, true);
break;
}
}
@@ -17273,12 +17277,12 @@ c_parser_oacc_routine (c_parser *parser, enum pragma_context context)
while (c_parser_next_token_is (parser, CPP_KEYWORD)
&& c_parser_peek_token (parser)->keyword == RID_EXTENSION);
c_parser_declaration_or_fndef (parser, true, true, true, false, true,
- NULL, vNULL, false, NULL, &data);
+ NULL, NULL, false, NULL, &data);
restore_extension_diagnostics (ext);
}
else
c_parser_declaration_or_fndef (parser, true, true, true, false, true,
- NULL, vNULL, false, NULL, &data);
+ NULL, NULL, false, NULL, &data);
}
}
@@ -18383,8 +18387,7 @@ c_parser_omp_for_loop (location_t loc, c_parser *parser, enum tree_code code,
vec_safe_push (for_block, c_begin_compound_stmt (true));
this_pre_body = push_stmt_list ();
c_in_omp_for = true;
- c_parser_declaration_or_fndef (parser, true, true, true, true, true,
- NULL, vNULL);
+ c_parser_declaration_or_fndef (parser, true, true, true, true, true);
c_in_omp_for = false;
if (this_pre_body)
{
@@ -20325,12 +20328,12 @@ c_parser_omp_declare_simd (c_parser *parser, enum pragma_context context)
while (c_parser_next_token_is (parser, CPP_KEYWORD)
&& c_parser_peek_token (parser)->keyword == RID_EXTENSION);
c_parser_declaration_or_fndef (parser, true, true, true, false, true,
- NULL, clauses);
+ NULL, &clauses);
restore_extension_diagnostics (ext);
}
else
c_parser_declaration_or_fndef (parser, true, true, true, false, true,
- NULL, clauses);
+ NULL, &clauses);
break;
case pragma_struct:
case pragma_param:
@@ -20351,7 +20354,7 @@ c_parser_omp_declare_simd (c_parser *parser, enum pragma_context context)
if (c_parser_next_tokens_start_declaration (parser))
{
c_parser_declaration_or_fndef (parser, true, true, true, true,
- true, NULL, clauses);
+ true, NULL, &clauses);
restore_extension_diagnostics (ext);
break;
}
@@ -20360,7 +20363,7 @@ c_parser_omp_declare_simd (c_parser *parser, enum pragma_context context)
else if (c_parser_next_tokens_start_declaration (parser))
{
c_parser_declaration_or_fndef (parser, true, true, true, true, true,
- NULL, clauses);
+ NULL, &clauses);
break;
}
error ("%<#pragma omp declare %s%> must be followed by "
@@ -20841,7 +20844,7 @@ c_finish_omp_declare_variant (c_parser *parser, tree fndecl, tree parms)
static void
c_finish_omp_declare_simd (c_parser *parser, tree fndecl, tree parms,
- vec<c_token> clauses)
+ vec<c_token> &clauses)
{
/* Normally first token is CPP_NAME "simd" or "variant". CPP_EOF there
indicates error has been reported and CPP_PRAGMA that
