Re: [PATCH] Fix type field walking in gimplifier unsharing

[Richard Biener <rguenther@suse.de>]

On Wed, 27 Apr 2016, Eric Botcazou wrote:

> > Gimplification is done in-place and thus relies on all processed
> > trees being unshared.  This is achieved by unshare_body which
> > in the end uses walk_tree to get at all interesting trees that possibly
> > need unsharing.
> > 
> > Unfortunately it doesn't really work because walk_tree only walks
> > types and type-related fields (TYPE_SIZE, TYPE_MIN_VALUE, etc.) in
> > very narrow circumstances.
> 
> Right, but well defined and explained:
> 
>     case DECL_EXPR:
>       /* If this is a TYPE_DECL, walk into the fields of the type that it's
> 	 defining.  We only want to walk into these fields of a type in this
> 	 case and not in the general case of a mere reference to the type.
> 
> 	 The criterion is as follows: if the field can be an expression, it
> 	 must be walked only here.  This should be in keeping with the fields
> 	 that are directly gimplified in gimplify_type_sizes in order for the
> 	 mark/copy-if-shared/unmark machinery of the gimplifier to work with
> 	 variable-sized types.
> 
> 	 Note that DECLs get walked as part of processing the BIND_EXPR.  */
> 
> > Thus the following patch which makes the gimplifier unsharing
> > visit all types.
> 
> I think this will generate a lot of useless walking in Ada...
> 
> > So - any opinion on the "correct" way to fix this?
> 
> Add DECL_EXPRs for the types, that's what done in Ada.

Aww, I was hoping for sth that would not require me to fix all
frontends ...

It seems the C frontend does it correctly already - I hit the
ubsan issue for c-c++-common/ubsan/pr59667.c and only for the C++ FE
for example.  Notice how only the pointed-to type is variable-size
here.

C produces

{
  unsigned int len = 1;
  typedef float <anon>[0:(sizetype) ((long int) SAVE_EXPR <len> + 
-1)][0:(sizetype) ((long int) SAVE_EXPR <len> + -1)];
  float[0:(sizetype) ((long int) SAVE_EXPR <len> + -1)][0:(sizetype) 
((long int) SAVE_EXPR <len> + -1)] * P = 0B;

    unsigned int len = 1;
    typedef float <anon>[0:(sizetype) ((long int) SAVE_EXPR <len> + 
-1)][0:(sizetype) ((long int) SAVE_EXPR <len> + -1)];
  SAVE_EXPR <len>;, (void) SAVE_EXPR <len>;;
    float[0:(sizetype) ((long int) SAVE_EXPR <len> + -1)][0:(sizetype) 
((long int) SAVE_EXPR <len> + -1)] * P = 0B;
  (*P)[0][0] = 1.0e+0;
  return 0;
}

the decl-expr is the 'typedef' line.  The C++ FE produces

{
  unsigned int len = 1;
  float[0:(sizetype) (SAVE_EXPR <(ssizetype) len + -1>)][0:(sizetype) 
(SAVE_EXPR <(ssizetype) len + -1>)] * P = 0B;

  <<cleanup_point   unsigned int len = 1;>>;
  <<cleanup_point <<< Unknown tree: expr_stmt
  (void) (((bitsizetype) ((sizetype) (SAVE_EXPR <(ssizetype) len + -1>) + 
1) * (bitsizetype) ((sizetype) (SAVE_EXPR <(ssizetype) len + -1>) + 1)) * 
32) >>>>>;
  <<cleanup_point   float[0:(sizetype) (SAVE_EXPR <(ssizetype) len + 
-1>)][0:(sizetype) (SAVE_EXPR <(ssizetype) len + -1>)] * P = 0B;>>;
  <<cleanup_point <<< Unknown tree: expr_stmt
  (void) ((*P)[0][0] = 1.0e+0) >>>>>;
  return <retval> = 0;
}

notice the lack of a decl-expr here.  It has some weird expr_stmt
here covering the sizes though. Possibly because VLA arrays are a GNU 
extension.

Didn't look into the fortran FE issue but I expect it's similar
(it only occurs for pointers to VLAs as well).

I'll try to come up with patches.

Thanks for the hint,
Richard.