Page MenuHomePhabricator

[CodeGen] Fix crash when a function taking transparent union is redeclared.

Authored by vsapsai on Dec 15 2017, 3:23 PM.



When a function taking transparent union is declared as taking one of
union members earlier in the translation unit, clang would hit an
"Invalid cast" assertion during EmitFunctionProlog. This case
corresponds to function f1 in test/CodeGen/transparent-union-redecl.c.
We decided to cast i32 to union because after merging function
declarations function parameter type becomes int,
CGFunctionInfo::ArgInfo type matches with ABIArgInfo type, so we decide
it is a trivial case. But these types should also be castable to
parameter declaration type which is not the case here.

The fix is in checking for the trivial case if we would be able to cast
to parameter declaration type. It exposed inconsistency that we check
hasScalarEvaluationKind for different types in EmitParmDecl and
EmitFunctionProlog, and comment says they should match.

Additional tests in Sema/transparent-union.c capture current behavior.
Added them because when trying different fix I regressed on those cases
but there were no tests to catch it.


Diff Detail

rC Clang

Event Timeline

vsapsai created this revision.Dec 15 2017, 3:23 PM
rjmccall added inline comments.Dec 15 2017, 7:00 PM
2321 ↗(On Diff #127199)

I think the right fix is to change the second clause to ArgI.getCoerceToType() == ConvertType(Arg->getType()). The point of this special case is to catch the common case that the natural way that IRGen wants to emit the argument expression is by producing a single scalar of exactly the right IR type; it seems to me that that condition would capture that correctly.

vsapsai added inline comments.Dec 19 2017, 6:44 PM
2321 ↗(On Diff #127199)

I've tried that and it almost works. There are 4 failing tests

Clang :: CodeGen/kr-func-promote.c
Clang :: CodeGenCXX/microsoft-abi-multiple-nonvirtual-inheritance.cpp
Clang :: CodeGenCXX/microsoft-abi-virtual-inheritance-vtordisps.cpp
Clang :: CodeGenCXX/microsoft-abi-virtual-inheritance.cpp

In a couple of places the problem is that expected function i32 @a(i32) but received i32 @a(i32 %x.coerce).

As for me, parameter naming is not really a problem and can be fixed by making tests less specific. What is more interesting, with the change we started adding extra store and load. For example, for CodeGen/kr-func-promote.c IR was

define i32 @a(i32) #0 {
  %x.addr = alloca i16, align 2
  %x = trunc i32 %0 to i16
  store i16 %x, i16* %x.addr, align 2
  %2 = load i16, i16* %x.addr, align 2
  %conv = sext i16 %2 to i32
  ret i32 %conv

and with type comparison change IR becomes

define i32 @a(i32 %x.coerce) #0 {
  %x = alloca i16, align 2
  %x.addr = alloca i16, align 2
  %coerce.val.ii = trunc i32 %x.coerce to i16
  store i16 %coerce.val.ii, i16* %x, align 2
  %x1 = load i16, i16* %x, align 2
  store i16 %x1, i16* %x.addr, align 2
  %0 = load i16, i16* %x.addr, align 2
  %conv = sext i16 %0 to i32
  ret i32 %conv

The same situation is with "microsoft-abi-*" tests where instead of

%this = bitcast i8* %0 to %struct.D*
store %struct.D* %this, %struct.D** %this.addr, align 4
%this1 = load %struct.D*, %struct.D** %this.addr, align 4
%2 = bitcast %struct.D* %this1 to i8*

we have

%coerce.val = bitcast i8* %this.coerce to %struct.D*
store %struct.D* %coerce.val, %struct.D** %this, align 4
%this1 = load %struct.D*, %struct.D** %this, align 4
store %struct.D* %this1, %struct.D** %this.addr, align 4
%this2 = load %struct.D*, %struct.D** %this.addr, align 4
%0 = bitcast %struct.D* %this2 to i8*

I'll check where and why we are adding extra store/load.

vsapsai added inline comments.Dec 20 2017, 11:52 AM
2321 ↗(On Diff #127199)

The last store comes from CodeGenFunction::EmitParmDecl where for direct arguments we have

if (Arg.isIndirect()) {
} else {
  // Otherwise, create a temporary to hold the value.
  DeclPtr = CreateMemTemp(Ty, getContext().getDeclAlign(&D),
                          D.getName() + ".addr");
  DoStore = true;

We have this store before and after my change, and it doesn't depend on comparing ArgI.getCoerceToType() to other types.

The extra store+load comes from CreateCoercedStore and EmitLoadOfScalar a few lines lower. I've tried to avoid this store+load for cases where we can use only emitArgumentDemotion and CreateBitCast. But it doesn't look to be particularly simple and starts to look like incomplete version of "trivial case, handle it with no muss and fuss".

Knowing the cause of the extra store+load I don't think it is crucial to avoid it. I'll update the patch with ArgI.getCoerceToType() == ConvertType(Arg->getType()) comparison and fixed tests.

2461 ↗(On Diff #127199)

Here is where we add the extra store.

vsapsai updated this revision to Diff 127779.Dec 20 2017, 1:12 PM
  • Address review comment, compare ArgI type with Arg type. Update tests to account for coerced store when types don't match.
vsapsai marked an inline comment as done.Dec 20 2017, 1:14 PM
rjmccall accepted this revision.Dec 20 2017, 7:16 PM

Okay, I think that's reasonable enough. LGTM.

This revision is now accepted and ready to land.Dec 20 2017, 7:16 PM
This revision was automatically updated to reflect the committed changes.
vsapsai reopened this revision.Jan 4 2018, 1:06 PM

The fix wasn't entirely correct and was reverted in r321306 as it caused test failures

FAIL: imp.execution_time
FAIL: 2007-01-04-KNR-Args.execution_time
FAIL: sse_expandfft.execution_time
FAIL: sse_stepfft.execution_time

in LLVM test suite. Will push new revision with better fix shortly.

This revision is now accepted and ready to land.Jan 4 2018, 1:06 PM
vsapsai updated this revision to Diff 128643.Jan 4 2018, 1:14 PM
  • Fix using CreateCoercedStore for promoted float parameters.

For promoted parameters double -> float conversion should use fptrunc, not
casting through memory preserving bits. Tried to change CreateCoercedStore to
do CreateFPCast but it caused CodeGen/arm-fp16-arguments.c to fail as it
expects __fp16 values to be passed in the least-significant 16 bits.

vsapsai requested review of this revision.Jan 4 2018, 1:15 PM

Add Akira as __fp16 expert to make sure I don't break some edge cases.

It doesn't look like this patch would break IRGen for functions that receive fp16 types. fp16 is passed as i32 or float (without promotion) on ARM and as half on ARM64. It should work fine.

ahatanak added inline comments.Jan 5 2018, 10:12 PM
2321 ↗(On Diff #128643)

Maybe a comment explaining why different types are passed depending on the value of isPromoted?

vsapsai added inline comments.Jan 10 2018, 4:34 PM
2321 ↗(On Diff #128643)

My reasoning was that for promoted arguments emitArgumentDemotion is responsible for bridging gaps between Ty and Arg->getType(), so it should be enough for ArgI.getCoerceToType() and Ty to match. But this explanation is woefully unsuitable for the comment.

After some more thinking I start doubting if some other s/Ty/Arg->getType()/ changes are entirely correct.

2469 ↗(On Diff #128643)

For example, here. If V is of type Arg->getType(), will emitArgumentDemotion do anything? Because there you have

if (value->getType() == varType) return value;

I'll keep digging and if meanwhile anybody has comments on this situation, I'll be glad to hear them.

vsapsai updated this revision to Diff 130038.Jan 16 2018, 2:45 PM
  • Try another approach for the fix. It has broader impact but semantically seems to be more correct.

Also I think hasScalarEvaluationKind should use Arg->getType() everywhere
as it matches EmitParmDecl behaviour. Instead of updating all
hasScalarEvaluationKind calls I've added an assertion to verify it doesn't
matter if we call it with Arg->getType() or Ty.

rjmccall accepted this revision.Jan 22 2018, 7:48 AM


This revision is now accepted and ready to land.Jan 22 2018, 7:48 AM
This revision was automatically updated to reflect the committed changes.