Skip to content

Commit

Permalink
Fix ICEs related to VM types in C [PR106465, PR107557, PR108423, PR10…
Browse files Browse the repository at this point in the history
…9450]

Thanks Joseph!

Revised version attached. Ok?

But I wonder whether we generally need to do something 
about

  sizeof *x

when x is NULL or not initialized. This is quite commonly
used in C code and if the type is not of variable size,
it is also unproblematic.  So the UB for variable size is
unfortunate and certainly also affects existing code in
the wild.  In practice it does not seem to cause
problems because there is no lvalue conversion and this
then seems to work.  Maybe we document this as an 
extension?  (and make sure in the C FE that it
works)  This would also make this idiom valid:

char (*buf)[n] = malloc(sizeof *buf);

Or if we do not want to do this, then I think we should
add some warnings (and UBSan check for null pointer)
which currently do not exist:

https://godbolt.org/z/fhWMKvYc8

Martin

Am Donnerstag, dem 18.05.2023 um 21:46 +0000 schrieb Joseph Myers:
> On Thu, 18 May 2023, Martin Uecker via Gcc-patches wrote:
>
> > +      /* we still have to evaluate size expressions */
>
> Comments should start with a capital letter and end with ".  ".
>
> > diff --git a/gcc/testsuite/gcc.dg/nested-vla-1.c b/gcc/testsuite/gcc.dg/nested-vla-1.c
> > new file mode 100644
> > index 00000000000..408a68524d8
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/nested-vla-1.c
> > @@ -0,0 +1,37 @@
> > +/* { dg-do run } */
> > +/* { dg-options "-std=gnu99" } */
>
> I'm concerned with various undefined behavior in this and other tests;
> they look very fragile, relying on some optimizations and not others
> taking place.  I think they should be adjusted to avoid undefined behavior
> if all the evaluations from the abstract machine (in particular, of sizeof
> operands with variable size) take place, and other undefined behavior from
> calling functions through function pointers with incompatible type.
>
> > +	struct bar { char x[++n]; } (*bar2)(void) = bar;	/* { dg-warning "incompatible pointer type" } */
> > +
> > +	if (2 != n)
> > +		__builtin_abort();
> > +
> > +	if (2 != sizeof((*bar2)()))
> > +		__builtin_abort();
>
> You're relying on the compiler not noticing that a function is being
> called through an incompatible type and thus not turning the call (which
> should be evaluated, because the operand of sizeof has a type with
> variable size) into a call to abort.
>
> > diff --git a/gcc/testsuite/gcc.dg/nested-vla-2.c b/gcc/testsuite/gcc.dg/nested-vla-2.c
> > new file mode 100644
> > index 00000000000..504eec48c80
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/nested-vla-2.c
> > @@ -0,0 +1,33 @@
> > +/* { dg-do run } */
> > +/* { dg-options "-std=gnu99" } */
> > +
> > +
> > +int main()
> > +{
> > +	int n = 1;
> > +
> > +	typeof(char (*)[++n]) bar(void) { }
> > +
> > +	if (2 != n)
> > +		__builtin_abort();
> > +
> > +	if (2 != sizeof(*bar()))
> > +		__builtin_abort();
>
> In this test, *bar() is evaluated, i.e. an undefined pointer is
> dereferenced; it would be better to return a valid pointer to a
> sufficiently large array to avoid that undefined behavior.
>
> > diff --git a/gcc/testsuite/gcc.dg/pr106465.c b/gcc/testsuite/gcc.dg/pr106465.c
> > new file mode 100644
> > index 00000000000..b03e2442f12
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/pr106465.c
> > @@ -0,0 +1,86 @@
> > +/* PR c/106465
> > + * { dg-do run }
> > + * { dg-options "-std=gnu99" }
> > + * */
> > +
> > +int main()
> > +{
> > +	int n = 3;
> > +
> > +	void g1(int m, struct { char p[++m]; }* b)	/* { dg-warning "anonymous struct" } */
> > +	{
> > +		if (3 != m)
> > +			__builtin_abort();
> > +
> > +		if (3 != sizeof(b->p))
> > +			__builtin_abort();
> > +	}
>
> > +	g1(2, (void*)0);
>
> Similarly, this is dereferencing a null pointer in the evaluated operand
> of sizeof.

>
  • Loading branch information
uecker authored and ouuleilei-bot committed May 19, 2023
1 parent c2d62cd commit 7f6359a
Show file tree
Hide file tree
Showing 26 changed files with 566 additions and 62 deletions.
39 changes: 22 additions & 17 deletions gcc/c/c-decl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5378,7 +5378,8 @@ start_decl (struct c_declarator *declarator, struct c_declspecs *declspecs,
if (lastdecl != error_mark_node)
*lastloc = DECL_SOURCE_LOCATION (lastdecl);

if (expr)
/* Make sure the size expression is evaluated at this point. */
if (expr && !current_scope->parm_flag)
add_stmt (fold_convert (void_type_node, expr));

if (TREE_CODE (decl) != FUNCTION_DECL && MAIN_NAME_P (DECL_NAME (decl))
Expand Down Expand Up @@ -7507,7 +7508,8 @@ grokdeclarator (const struct c_declarator *declarator,
&& c_type_variably_modified_p (type))
{
tree bind = NULL_TREE;
if (decl_context == TYPENAME || decl_context == PARM)
if (decl_context == TYPENAME || decl_context == PARM
|| decl_context == FIELD)
{
bind = build3 (BIND_EXPR, void_type_node, NULL_TREE,
NULL_TREE, NULL_TREE);
Expand All @@ -7516,10 +7518,11 @@ grokdeclarator (const struct c_declarator *declarator,
push_scope ();
}
tree decl = build_decl (loc, TYPE_DECL, NULL_TREE, type);
DECL_ARTIFICIAL (decl) = 1;
pushdecl (decl);
finish_decl (decl, loc, NULL_TREE, NULL_TREE, NULL_TREE);
DECL_ARTIFICIAL (decl) = 1;
add_stmt (build_stmt (DECL_SOURCE_LOCATION (decl), DECL_EXPR, decl));
TYPE_NAME (type) = decl;

if (bind)
{
pop_scope ();
Expand Down Expand Up @@ -8718,7 +8721,7 @@ start_struct (location_t loc, enum tree_code code, tree name,
tree
grokfield (location_t loc,
struct c_declarator *declarator, struct c_declspecs *declspecs,
tree width, tree *decl_attrs)
tree width, tree *decl_attrs, tree *expr)
{
tree value;

Expand Down Expand Up @@ -8775,7 +8778,7 @@ grokfield (location_t loc,
}

value = grokdeclarator (declarator, declspecs, FIELD, false,
width ? &width : NULL, decl_attrs, NULL, NULL,
width ? &width : NULL, decl_attrs, expr, NULL,
DEPRECATED_NORMAL);

finish_decl (value, loc, NULL_TREE, NULL_TREE, NULL_TREE);
Expand Down Expand Up @@ -9433,13 +9436,6 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,

finish_incomplete_vars (incomplete_vars, toplevel);

/* If we're inside a function proper, i.e. not file-scope and not still
parsing parameters, then arrange for the size of a variable sized type
to be bound now. */
if (building_stmt_list_p () && c_type_variably_modified_p(t))
add_stmt (build_stmt (loc,
DECL_EXPR, build_decl (loc, TYPE_DECL, NULL, t)));

if (warn_cxx_compat)
warn_cxx_compat_finish_struct (fieldlist, TREE_CODE (t), loc);

Expand Down Expand Up @@ -10065,6 +10061,7 @@ start_function (struct c_declspecs *declspecs, struct c_declarator *declarator,
tree restype, resdecl;
location_t loc;
location_t result_loc;
tree expr = NULL;

current_function_returns_value = 0; /* Assume, until we see it does. */
current_function_returns_null = 0;
Expand All @@ -10076,7 +10073,7 @@ start_function (struct c_declspecs *declspecs, struct c_declarator *declarator,
in_statement = 0;

decl1 = grokdeclarator (declarator, declspecs, FUNCDEF, true, NULL,
&attributes, NULL, NULL, DEPRECATED_NORMAL);
&attributes, &expr, NULL, DEPRECATED_NORMAL);
invoke_plugin_callbacks (PLUGIN_START_PARSE_FUNCTION, decl1);

/* If the declarator is not suitable for a function definition,
Expand All @@ -10085,6 +10082,11 @@ start_function (struct c_declspecs *declspecs, struct c_declarator *declarator,
|| TREE_CODE (decl1) != FUNCTION_DECL)
return false;

/* Nested functions may have variably modified (return) type.
Make sure the size expression is evaluated at this point. */
if (expr && !current_scope->parm_flag)
add_stmt (fold_convert (void_type_node, expr));

loc = DECL_SOURCE_LOCATION (decl1);

/* A nested function is not global. */
Expand Down Expand Up @@ -12289,10 +12291,13 @@ declspecs_add_type (location_t loc, struct c_declspecs *specs,
}
else
{
if (TREE_CODE (type) != ERROR_MARK && spec.kind == ctsk_typeof)
if (TREE_CODE (type) != ERROR_MARK)
{
specs->typedef_p = true;
specs->locations[cdw_typedef] = loc;
if (spec.kind == ctsk_typeof)
{
specs->typedef_p = true;
specs->locations[cdw_typedef] = loc;
}
if (spec.expr)
{
if (specs->expr)
Expand Down
22 changes: 13 additions & 9 deletions gcc/c/c-parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1541,7 +1541,7 @@ static void c_parser_static_assert_declaration_no_semi (c_parser *);
static void c_parser_static_assert_declaration (c_parser *);
static struct c_typespec c_parser_enum_specifier (c_parser *);
static struct c_typespec c_parser_struct_or_union_specifier (c_parser *);
static tree c_parser_struct_declaration (c_parser *);
static tree c_parser_struct_declaration (c_parser *, tree *);
static struct c_typespec c_parser_typeof_specifier (c_parser *);
static tree c_parser_alignas_specifier (c_parser *);
static struct c_declarator *c_parser_direct_declarator (c_parser *, bool,
Expand Down Expand Up @@ -2263,6 +2263,9 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
if (!handled_assume)
pedwarn (here, 0, "empty declaration");
}
/* We still have to evaluate size expressions. */
if (specs->expr)
add_stmt (fold_convert (void_type_node, specs->expr));
c_parser_consume_token (parser);
if (oacc_routine_data)
c_finish_oacc_routine (oacc_routine_data, NULL_TREE, false);
Expand Down Expand Up @@ -3782,6 +3785,7 @@ c_parser_struct_or_union_specifier (c_parser *parser)
so we'll be minimizing the number of node traversals required
by chainon. */
tree contents;
tree expr = NULL;
timevar_push (TV_PARSE_STRUCT);
contents = NULL_TREE;
c_parser_consume_token (parser);
Expand Down Expand Up @@ -3843,7 +3847,7 @@ c_parser_struct_or_union_specifier (c_parser *parser)
}
/* Parse some comma-separated declarations, but not the
trailing semicolon if any. */
decls = c_parser_struct_declaration (parser);
decls = c_parser_struct_declaration (parser, &expr);
contents = chainon (decls, contents);
/* If no semicolon follows, either we have a parse error or
are at the end of the struct or union and should
Expand Down Expand Up @@ -3874,7 +3878,7 @@ c_parser_struct_or_union_specifier (c_parser *parser)
chainon (attrs, postfix_attrs)),
struct_info);
ret.kind = ctsk_tagdef;
ret.expr = NULL_TREE;
ret.expr = expr;
ret.expr_const_operands = true;
ret.has_enum_type_specifier = false;
timevar_pop (TV_PARSE_STRUCT);
Expand Down Expand Up @@ -3936,7 +3940,7 @@ c_parser_struct_or_union_specifier (c_parser *parser)
expressions will be diagnosed as non-constant. */

static tree
c_parser_struct_declaration (c_parser *parser)
c_parser_struct_declaration (c_parser *parser, tree *expr)
{
struct c_declspecs *specs;
tree prefix_attrs;
Expand All @@ -3949,7 +3953,7 @@ c_parser_struct_declaration (c_parser *parser)
tree decl;
ext = disable_extension_diagnostics ();
c_parser_consume_token (parser);
decl = c_parser_struct_declaration (parser);
decl = c_parser_struct_declaration (parser, expr);
restore_extension_diagnostics (ext);
return decl;
}
Expand Down Expand Up @@ -3995,7 +3999,7 @@ c_parser_struct_declaration (c_parser *parser)

ret = grokfield (c_parser_peek_token (parser)->location,
build_id_declarator (NULL_TREE), specs,
NULL_TREE, &attrs);
NULL_TREE, &attrs, expr);
if (ret)
decl_attributes (&ret, attrs, 0);
}
Expand Down Expand Up @@ -4056,7 +4060,7 @@ c_parser_struct_declaration (c_parser *parser)
if (c_parser_next_token_is_keyword (parser, RID_ATTRIBUTE))
postfix_attrs = c_parser_gnu_attributes (parser);
d = grokfield (c_parser_peek_token (parser)->location,
declarator, specs, width, &all_prefix_attrs);
declarator, specs, width, &all_prefix_attrs, expr);
decl_attributes (&d, chainon (postfix_attrs,
all_prefix_attrs), 0);
DECL_CHAIN (d) = decls;
Expand Down Expand Up @@ -11732,7 +11736,7 @@ c_parser_objc_class_instance_variables (c_parser *parser)
}

/* Parse some comma-separated declarations. */
decls = c_parser_struct_declaration (parser);
decls = c_parser_struct_declaration (parser, NULL);
if (decls == NULL)
{
/* There is a syntax error. We want to skip the offending
Expand Down Expand Up @@ -12871,7 +12875,7 @@ c_parser_objc_at_property_declaration (c_parser *parser)
/* 'properties' is the list of properties that we read. Usually a
single one, but maybe more (eg, in "@property int a, b, c;" there
are three). */
tree properties = c_parser_struct_declaration (parser);
tree properties = c_parser_struct_declaration (parser, NULL);

if (properties == error_mark_node)
c_parser_skip_until_found (parser, CPP_SEMICOLON, NULL);
Expand Down
2 changes: 1 addition & 1 deletion gcc/c/c-tree.h
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ extern tree c_simulate_record_decl (location_t, const char *,
extern struct c_arg_info *build_arg_info (void);
extern struct c_arg_info *get_parm_info (bool, tree);
extern tree grokfield (location_t, struct c_declarator *,
struct c_declspecs *, tree, tree *);
struct c_declspecs *, tree, tree *, tree *);
extern tree groktypename (struct c_type_name *, tree *, bool *);
extern tree grokparm (const struct c_parm *, tree *);
extern tree implicitly_declare (location_t, tree);
Expand Down
33 changes: 1 addition & 32 deletions gcc/function.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3872,30 +3872,6 @@ assign_parms (tree fndecl)
}
}

/* A subroutine of gimplify_parameters, invoked via walk_tree.
For all seen types, gimplify their sizes. */

static tree
gimplify_parm_type (tree *tp, int *walk_subtrees, void *data)
{
tree t = *tp;

*walk_subtrees = 0;
if (TYPE_P (t))
{
if (POINTER_TYPE_P (t))
*walk_subtrees = 1;
else if (TYPE_SIZE (t) && !TREE_CONSTANT (TYPE_SIZE (t))
&& !TYPE_SIZES_GIMPLIFIED (t))
{
gimplify_type_sizes (t, (gimple_seq *) data);
*walk_subtrees = 1;
}
}

return NULL;
}

/* Gimplify the parameter list for current_function_decl. This involves
evaluating SAVE_EXPRs of variable sized parameters and generating code
to implement callee-copies reference parameters. Returns a sequence of
Expand Down Expand Up @@ -3931,14 +3907,7 @@ gimplify_parameters (gimple_seq *cleanup)
SAVE_EXPRs (amongst others) onto a pending sizes list. This
turned out to be less than manageable in the gimple world.
Now we have to hunt them down ourselves. */
walk_tree_without_duplicates (&data.arg.type,
gimplify_parm_type, &stmts);

if (TREE_CODE (DECL_SIZE_UNIT (parm)) != INTEGER_CST)
{
gimplify_one_sizepos (&DECL_SIZE (parm), &stmts);
gimplify_one_sizepos (&DECL_SIZE_UNIT (parm), &stmts);
}
gimplify_parm_sizes (parm, &stmts);

if (data.arg.pass_by_reference)
{
Expand Down
18 changes: 17 additions & 1 deletion gcc/gimplify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ static struct gimplify_omp_ctx *gimplify_omp_ctxp;
static bool in_omp_construct;

/* Forward declaration. */
static void gimplify_type_sizes (tree type, gimple_seq *list_p);
static enum gimplify_status gimplify_compound_expr (tree *, gimple_seq *, bool);
static hash_map<tree, tree> *oacc_declare_returns;
static enum gimplify_status gimplify_expr (tree *, gimple_seq *, gimple_seq *,
Expand Down Expand Up @@ -17467,7 +17468,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
/* Look through TYPE for variable-sized objects and gimplify each such
size that we find. Add to LIST_P any statements generated. */

void
static void
gimplify_type_sizes (tree type, gimple_seq *list_p)
{
if (type == NULL || type == error_mark_node)
Expand Down Expand Up @@ -17575,6 +17576,21 @@ gimplify_type_sizes (tree type, gimple_seq *list_p)
}
}

/* Gimplify sizes in parameter declarations. */

void
gimplify_parm_sizes (tree parm, gimple_seq *list_p)
{
gimplify_type_sizes (TREE_TYPE (parm), list_p);

if (TREE_CODE (DECL_SIZE_UNIT (parm)) != INTEGER_CST)
{
gimplify_one_sizepos (&DECL_SIZE (parm), list_p);
gimplify_one_sizepos (&DECL_SIZE_UNIT (parm), list_p);
}
}


/* A subroutine of gimplify_type_sizes to make sure that *EXPR_P,
a size or position, has had all of its SAVE_EXPRs evaluated.
We add any required statements to *STMT_P. */
Expand Down
2 changes: 1 addition & 1 deletion gcc/gimplify.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ extern enum gimplify_status gimplify_expr (tree *, gimple_seq *, gimple_seq *,

int omp_construct_selector_matches (enum tree_code *, int, int *);

extern void gimplify_type_sizes (tree, gimple_seq *);
extern void gimplify_parm_sizes (tree, gimple_seq *);
extern void gimplify_one_sizepos (tree *, gimple_seq *);
extern gbind *gimplify_body (tree, bool);
extern enum gimplify_status gimplify_arg (tree *, gimple_seq *, location_t,
Expand Down
63 changes: 63 additions & 0 deletions gcc/testsuite/gcc.dg/nested-vla-1.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/* { dg-do run } */
/* { dg-options "-std=gnu99" } */


int main()
{
int n = 1;

struct foo { char x[++n]; } bar(void) { struct foo r; return r; }

if (2 != n)
__builtin_abort();

if (2 != sizeof(bar()))
__builtin_abort();

n = 1;

struct bar { char x[++n]; } (*bar2p)(void);
struct bar bar2(void) { struct bar r; return r; }
bar2p = &bar2;

if (2 != n)
__builtin_abort();

if (2 != sizeof((*bar2p)()))
__builtin_abort();

n = 1;

struct str { char x[++n]; } *bar3(void)
{
struct str* s = __builtin_malloc(sizeof(struct str));
if (!s)
__builtin_abort();
struct str t;
*s = t;
return s;
}

if (2 != n)
__builtin_abort();

struct str* p;

if (2 != sizeof(*(p = bar3())))
__builtin_abort();

__builtin_free(p);

n = 1;

struct { char x[++n]; } *bar4(void) { }

if (2 != n)
__builtin_abort();
#if 0
// UB
if (2 != sizeof(*bar4()))
__builtin_abort();
#endif
}

Loading

0 comments on commit 7f6359a

Please sign in to comment.