Preserve static type info for return value of ctor

[sbomer]

Fixes#101102.

Instead of tracking the return value as "unknown", this models the constructor as returning a value with a static type, letting us undo the workaround from #101031.

[jtschuster]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

runtime/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs

Lines 1107 to 1119 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction){
	if(isNewObj){
	if(newObjValue==null)
	methodReturnValue=newMultiValue(UnknownValue.Instance);
	else
	methodReturnValue=newObjValue;
	}else{
	if(!calledMethod.ReturnsVoid()){
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs

Lines 1302 to 1319 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction)
	{
	if(isNewObj)
	{
	if(newObjValue==null)
	methodReturnValue=UnknownValue.Instance;
	else
	methodReturnValue=newObjValue;
	}
	else
	{
	if(!calledMethod.Signature.ReturnType.IsVoid)
	{
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[sbomer]

On the linker side I'm seeing some cases where handledFunction could potentially be false for a call to a ctor. Maybe it could be cleaned up, but I wouldn't do it as part of this change. @MichalStrehovsky any opinion on that?

[jtschuster]

Assuming you tested it on the code that caused the original issue, LGTM.

[MichalStrehovsky]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

If we want to remove that, I'd remove the whole handledFunction part and the bool return value that feeds it and require the method to handle it always.

We have too many codepaths that try to manufacture a default return value. There's the one this PR is changing on line 1170 of HandleCallAction, but we also have these:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

Lines 778 to 784 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.Signature.ReturnType.IsVoid;
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

runtime/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

Lines 355 to 361 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.ReturnsVoid();
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

I wonder if we should change those too.

Ideally there would only be one place that manufactures these and not 3.

This place has the most context to do the right thing though (it is the place where we see it's a constructor, and the instruction was newobj - we lose the newobj information in the other spots).

[sbomer]

Fixes#101102.

Instead of tracking the return value as "unknown", this models the constructor as returning a value with a static type, letting us undo the workaround from #101031.

[jtschuster]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

runtime/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs

Lines 1107 to 1119 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction){
	if(isNewObj){
	if(newObjValue==null)
	methodReturnValue=newMultiValue(UnknownValue.Instance);
	else
	methodReturnValue=newObjValue;
	}else{
	if(!calledMethod.ReturnsVoid()){
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs

Lines 1302 to 1319 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction)
	{
	if(isNewObj)
	{
	if(newObjValue==null)
	methodReturnValue=UnknownValue.Instance;
	else
	methodReturnValue=newObjValue;
	}
	else
	{
	if(!calledMethod.Signature.ReturnType.IsVoid)
	{
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[sbomer]

On the linker side I'm seeing some cases where handledFunction could potentially be false for a call to a ctor. Maybe it could be cleaned up, but I wouldn't do it as part of this change. @MichalStrehovsky any opinion on that?

[jtschuster]

Assuming you tested it on the code that caused the original issue, LGTM.

[MichalStrehovsky]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

If we want to remove that, I'd remove the whole handledFunction part and the bool return value that feeds it and require the method to handle it always.

We have too many codepaths that try to manufacture a default return value. There's the one this PR is changing on line 1170 of HandleCallAction, but we also have these:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

Lines 778 to 784 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.Signature.ReturnType.IsVoid;
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

runtime/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

Lines 355 to 361 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.ReturnsVoid();
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

I wonder if we should change those too.

Ideally there would only be one place that manufactures these and not 3.

This place has the most context to do the right thing though (it is the place where we see it's a constructor, and the instruction was newobj - we lose the newobj information in the other spots).

[sbomer]

Fixes#101102.

Instead of tracking the return value as "unknown", this models the constructor as returning a value with a static type, letting us undo the workaround from #101031.

[jtschuster]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

runtime/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs

Lines 1107 to 1119 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction){
	if(isNewObj){
	if(newObjValue==null)
	methodReturnValue=newMultiValue(UnknownValue.Instance);
	else
	methodReturnValue=newObjValue;
	}else{
	if(!calledMethod.ReturnsVoid()){
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs

Lines 1302 to 1319 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction)
	{
	if(isNewObj)
	{
	if(newObjValue==null)
	methodReturnValue=UnknownValue.Instance;
	else
	methodReturnValue=newObjValue;
	}
	else
	{
	if(!calledMethod.Signature.ReturnType.IsVoid)
	{
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[sbomer]

On the linker side I'm seeing some cases where handledFunction could potentially be false for a call to a ctor. Maybe it could be cleaned up, but I wouldn't do it as part of this change. @MichalStrehovsky any opinion on that?

[jtschuster]

Assuming you tested it on the code that caused the original issue, LGTM.

[MichalStrehovsky]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

If we want to remove that, I'd remove the whole handledFunction part and the bool return value that feeds it and require the method to handle it always.

We have too many codepaths that try to manufacture a default return value. There's the one this PR is changing on line 1170 of HandleCallAction, but we also have these:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

Lines 778 to 784 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.Signature.ReturnType.IsVoid;
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

runtime/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

Lines 355 to 361 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.ReturnsVoid();
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

I wonder if we should change those too.

Ideally there would only be one place that manufactures these and not 3.

This place has the most context to do the right thing though (it is the place where we see it's a constructor, and the instruction was newobj - we lose the newobj information in the other spots).

[sbomer]

Fixes#101102.

Instead of tracking the return value as "unknown", this models the constructor as returning a value with a static type, letting us undo the workaround from #101031.

[jtschuster]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

runtime/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs

Lines 1107 to 1119 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction){
	if(isNewObj){
	if(newObjValue==null)
	methodReturnValue=newMultiValue(UnknownValue.Instance);
	else
	methodReturnValue=newObjValue;
	}else{
	if(!calledMethod.ReturnsVoid()){
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs

Lines 1302 to 1319 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction)
	{
	if(isNewObj)
	{
	if(newObjValue==null)
	methodReturnValue=UnknownValue.Instance;
	else
	methodReturnValue=newObjValue;
	}
	else
	{
	if(!calledMethod.Signature.ReturnType.IsVoid)
	{
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[sbomer]

On the linker side I'm seeing some cases where handledFunction could potentially be false for a call to a ctor. Maybe it could be cleaned up, but I wouldn't do it as part of this change. @MichalStrehovsky any opinion on that?

[jtschuster]

Assuming you tested it on the code that caused the original issue, LGTM.

[MichalStrehovsky]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

If we want to remove that, I'd remove the whole handledFunction part and the bool return value that feeds it and require the method to handle it always.

We have too many codepaths that try to manufacture a default return value. There's the one this PR is changing on line 1170 of HandleCallAction, but we also have these:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

Lines 778 to 784 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.Signature.ReturnType.IsVoid;
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

runtime/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

Lines 355 to 361 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.ReturnsVoid();
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

I wonder if we should change those too.

Ideally there would only be one place that manufactures these and not 3.

This place has the most context to do the right thing though (it is the place where we see it's a constructor, and the instruction was newobj - we lose the newobj information in the other spots).

[sbomer]

Fixes#101102.

Instead of tracking the return value as "unknown", this models the constructor as returning a value with a static type, letting us undo the workaround from #101031.

[jtschuster]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

runtime/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs

Lines 1107 to 1119 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction){
	if(isNewObj){
	if(newObjValue==null)
	methodReturnValue=newMultiValue(UnknownValue.Instance);
	else
	methodReturnValue=newObjValue;
	}else{
	if(!calledMethod.ReturnsVoid()){
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs

Lines 1302 to 1319 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction)
	{
	if(isNewObj)
	{
	if(newObjValue==null)
	methodReturnValue=UnknownValue.Instance;
	else
	methodReturnValue=newObjValue;
	}
	else
	{
	if(!calledMethod.Signature.ReturnType.IsVoid)
	{
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[sbomer]

On the linker side I'm seeing some cases where handledFunction could potentially be false for a call to a ctor. Maybe it could be cleaned up, but I wouldn't do it as part of this change. @MichalStrehovsky any opinion on that?

[jtschuster]

Assuming you tested it on the code that caused the original issue, LGTM.

[MichalStrehovsky]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

If we want to remove that, I'd remove the whole handledFunction part and the bool return value that feeds it and require the method to handle it always.

We have too many codepaths that try to manufacture a default return value. There's the one this PR is changing on line 1170 of HandleCallAction, but we also have these:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

Lines 778 to 784 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.Signature.ReturnType.IsVoid;
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

runtime/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

Lines 355 to 361 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.ReturnsVoid();
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

I wonder if we should change those too.

Ideally there would only be one place that manufactures these and not 3.

This place has the most context to do the right thing though (it is the place where we see it's a constructor, and the instruction was newobj - we lose the newobj information in the other spots).

[sbomer]

Fixes#101102.

Instead of tracking the return value as "unknown", this models the constructor as returning a value with a static type, letting us undo the workaround from #101031.

[jtschuster]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

runtime/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs

Lines 1107 to 1119 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction){
	if(isNewObj){
	if(newObjValue==null)
	methodReturnValue=newMultiValue(UnknownValue.Instance);
	else
	methodReturnValue=newObjValue;
	}else{
	if(!calledMethod.ReturnsVoid()){
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs

Lines 1302 to 1319 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction)
	{
	if(isNewObj)
	{
	if(newObjValue==null)
	methodReturnValue=UnknownValue.Instance;
	else
	methodReturnValue=newObjValue;
	}
	else
	{
	if(!calledMethod.Signature.ReturnType.IsVoid)
	{
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[sbomer]

On the linker side I'm seeing some cases where handledFunction could potentially be false for a call to a ctor. Maybe it could be cleaned up, but I wouldn't do it as part of this change. @MichalStrehovsky any opinion on that?

[jtschuster]

Assuming you tested it on the code that caused the original issue, LGTM.

[MichalStrehovsky]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

If we want to remove that, I'd remove the whole handledFunction part and the bool return value that feeds it and require the method to handle it always.

We have too many codepaths that try to manufacture a default return value. There's the one this PR is changing on line 1170 of HandleCallAction, but we also have these:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

Lines 778 to 784 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.Signature.ReturnType.IsVoid;
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

runtime/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

Lines 355 to 361 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.ReturnsVoid();
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

I wonder if we should change those too.

Ideally there would only be one place that manufactures these and not 3.

This place has the most context to do the right thing though (it is the place where we see it's a constructor, and the instruction was newobj - we lose the newobj information in the other spots).

[sbomer]

Fixes#101102.

Instead of tracking the return value as "unknown", this models the constructor as returning a value with a static type, letting us undo the workaround from #101031.

[jtschuster]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

runtime/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs

Lines 1107 to 1119 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction){
	if(isNewObj){
	if(newObjValue==null)
	methodReturnValue=newMultiValue(UnknownValue.Instance);
	else
	methodReturnValue=newObjValue;
	}else{
	if(!calledMethod.ReturnsVoid()){
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs

Lines 1302 to 1319 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction)
	{
	if(isNewObj)
	{
	if(newObjValue==null)
	methodReturnValue=UnknownValue.Instance;
	else
	methodReturnValue=newObjValue;
	}
	else
	{
	if(!calledMethod.Signature.ReturnType.IsVoid)
	{
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[sbomer]

On the linker side I'm seeing some cases where handledFunction could potentially be false for a call to a ctor. Maybe it could be cleaned up, but I wouldn't do it as part of this change. @MichalStrehovsky any opinion on that?

[jtschuster]

Assuming you tested it on the code that caused the original issue, LGTM.

[MichalStrehovsky]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

If we want to remove that, I'd remove the whole handledFunction part and the bool return value that feeds it and require the method to handle it always.

We have too many codepaths that try to manufacture a default return value. There's the one this PR is changing on line 1170 of HandleCallAction, but we also have these:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

Lines 778 to 784 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.Signature.ReturnType.IsVoid;
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

runtime/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

Lines 355 to 361 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.ReturnsVoid();
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

I wonder if we should change those too.

Ideally there would only be one place that manufactures these and not 3.

This place has the most context to do the right thing though (it is the place where we see it's a constructor, and the instruction was newobj - we lose the newobj information in the other spots).

[sbomer]

Fixes#101102.

Instead of tracking the return value as "unknown", this models the constructor as returning a value with a static type, letting us undo the workaround from #101031.

[jtschuster]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

runtime/src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs

Lines 1107 to 1119 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction){
	if(isNewObj){
	if(newObjValue==null)
	methodReturnValue=newMultiValue(UnknownValue.Instance);
	else
	methodReturnValue=newObjValue;
	}else{
	if(!calledMethod.ReturnsVoid()){
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs

Lines 1302 to 1319 in b35b67d

	// Handle the return value or newobj result
	if(!handledFunction)
	{
	if(isNewObj)
	{
	if(newObjValue==null)
	methodReturnValue=UnknownValue.Instance;
	else
	methodReturnValue=newObjValue;
	}
	else
	{
	if(!calledMethod.Signature.ReturnType.IsVoid)
	{
	methodReturnValue=UnknownValue.Instance;
	}
	}
	}

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[sbomer]

On the linker side I'm seeing some cases where handledFunction could potentially be false for a call to a ctor. Maybe it could be cleaned up, but I wouldn't do it as part of this change. @MichalStrehovsky any opinion on that?

[jtschuster]

Assuming you tested it on the code that caused the original issue, LGTM.

[MichalStrehovsky]

With these changes, are we able to remove the isNewObj check in MethodBodyScanner in the trimmer and ilc?

If we want to remove that, I'd remove the whole handledFunction part and the bool return value that feeds it and require the method to handle it always.

We have too many codepaths that try to manufacture a default return value. There's the one this PR is changing on line 1170 of HandleCallAction, but we also have these:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs

Lines 778 to 784 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.Signature.ReturnType.IsVoid;
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

runtime/src/tools/illink/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

Lines 355 to 361 in 65f6a08

	// If we get here, we handled this as an intrinsic. As a convenience, if the code above
	// didn't set the return value (and the method has a return value), we will set it to be an
	// unknown value with the return type of the method.
	boolreturnsVoid=calledMethod.ReturnsVoid();
	methodReturnValue=maybeMethodReturnValue??(returnsVoid?
	MultiValueLattice.Top:
	annotatedMethodReturnValue);

I wonder if we should change those too.

Ideally there would only be one place that manufactures these and not 3.

This place has the most context to do the right thing though (it is the place where we see it's a constructor, and the instruction was newobj - we lose the newobj information in the other spots).