[llvm-commits] [126488] Fix PR1352 and CFrontend/2007-04-24-VolatileStructCopy.c

[Duncan Sands]

Hi Chris,

>      // Non-bitfield aggregate value.
> -    Emit(TREE_OPERAND(exp, 1), LV.Ptr);
> -    if (DestLoc)
> +    if (DestLoc) {
> +      Emit(TREE_OPERAND(exp, 1), LV.Ptr);
>        EmitAggregateCopy(DestLoc, LV.Ptr, TREE_TYPE(exp), isVolatile, false);

why is this ^^^ right?  Doesn't a non-null DestLoc mean you are doing something
like: A = (B = C), where DestLoc represents A, and the current MODIFY_EXPR
represents B=C?  If so, then the above is bogus: isVolatile says whether writes
to B are volatile, not writes to A: if isVolatile is true, then you are doing
a volatile write to A, which may or may not need it, and a non-volatile write
to B when a volatile write is needed!

> +    } else if (!isVolatile) {
> +      Emit(TREE_OPERAND(exp, 1), LV.Ptr);
> +    } else {
> +      // Need to do a volatile store into TREE_OPERAND(exp, 1).  To do this, we
> +      // emit it into a temporary memory location, them do a volatile copy into

them -> then

> +      // the real destination.  This is probably suboptimal in some cases, but
> +      // it gets the volatile memory access right.  It would be better if the
> +      // destloc pointer of 'Emit' had a flag that indicated it should be
> +      // volatile.
> +      Value *Tmp = CreateTemporary(ConvertType(TREE_TYPE(TREE_OPERAND(exp,1))));
> +      Emit(TREE_OPERAND(exp, 1), Tmp);
> +      EmitAggregateCopy(LV.Ptr, Tmp, TREE_TYPE(TREE_OPERAND(exp,1)),
> +                        isVolatile, false);
> +    }
>      return 0;
>    }

Ciao,

Duncan.