[clang] Do not crash for unsupported fixed point to floating point conversion

[gousemoodhin]

Summary

• Fixed point to floating point conversion is unimplemented.
• If one of the operands has a floating type and the other operand has a fixed-point type, the function handleFloatConversion() is called because one of the operands has a floating type, but we do not handle fixed point type in this function (Implementation of fixed point to floating point conversion is missing), due to this compiler crashes. In order to avoid compiler crash, when one of the operands has a floating type and the other operand has a fixed-point type, return NULL.
• FIXME: Implementation of fixed point to floating point conversion.
• I am going to resolve FIXME in followup patches.
• Add the test case.

[ebevhan]

I think a more apt subject would be "[clang] Handle floating point to fixed-point conversion crash." The commit message should also refer back to the original Bugzilla ticket.

A test case which exhibits the behavior should also be added. test/Frontend/fixed_point_errors.c is probably a good place for that.

clang/lib/Sema/SemaExpr.cpp

1508

As I mentioned per email, this is not quite right. Floating-fixedpoint conversion is simply unimplemented.

The comment should probably contain a FIXME referring to this.

1512

I think this check would be better placed inside handleFloatConversion.

Or is it necessary that it is checked before handleComplexFloatConversion?

[gousemoodhin]

clang/lib/Sema/SemaExpr.cpp

1508

Reworked on comments, uploaded a new patch.

1512

Yes, it is better to place inside handleFloatConversion().

[ebevhan]

You need a test case for this, Frontend/fixed_point_errors.c is probably a good place for it.

[gousemoodhin]

In D81904#2199477, @ebevhan wrote:

You need a test case for this, Frontend/fixed_point_errors.c is probably a good place for it.

Done.

[ebevhan]

clang/test/Frontend/fixed_point_errors.c

257 ↗

(On Diff #283648)

Does this pass? This should be producing an error, which -verify will choke on. You'll need an expected-error comment similar to the other ones in the file.

[gousemoodhin]

clang/test/Frontend/fixed_point_errors.c

257 ↗

(On Diff #283648)

Hello ebevhan,
Just want to confirm regarding the test case interface, I have resolved the compiler crash issue, after resolving the crash issue, the compiler generates an error instead of crash. So, as per my understanding, the test case should check the crash issue.

Let me know if I incorrectly interpreted it.

[gousemoodhin]

clang/test/Frontend/fixed_point_errors.c

257 ↗

(On Diff #283648)

Done

[ebevhan]

The patch looks good now, if you can provide a more descriptive summary/commit message, I can commit it for you.

clang/test/Frontend/fixed_point_errors.c

257 ↗

(On Diff #283648)

Just want to confirm regarding the test case interface, I have resolved the compiler crash issue, after resolving the crash issue, the compiler generates an error instead of crash. So, as per my understanding, the test case should check the crash issue.

This is correct, but if diagnostics tests like fixed_point_errors.c produce diagnostics that aren't described in the file, the test will fail.

It looks good now.

[gousemoodhin]

This comment was removed by gousemoodhin.

[ebevhan]

I don't think "Handle fixed point conversion" is a descriptive enough title.

I am going to resolve FIXME in followup patches.

I assume by this you mean to move forward with adding floating-fixed conversions? I have an unsubmitted patch for APFixedPoint which implements the semantics of the conversions, but I'm currently waiting on D85312, D85314, and more recently I found D54749 which is probably a prerequisite for the IR codegen.

[gousemoodhin]

In D81904#2207262, @ebevhan wrote:

I don't think "Handle fixed point conversion" is a descriptive enough title.

I am going to resolve FIXME in followup patches.

I assume by this you mean to move forward with adding floating-fixed conversions? I have an unsubmitted patch for APFixedPoint which implements the semantics of the conversions, but I'm currently waiting on D85312, D85314, and more recently I found D54749 which is probably a prerequisite for the IR codegen.

Yes,
I will add a function for fixed point to floating point conversion, (Example: handleFixedPointToFloatConversion()), similar to handleIntToFloatConversion() (SemaExpr.cpp)

[ebevhan]

In D81904#2207335, @gousemoodhin wrote:

In D81904#2207262, @ebevhan wrote:

I don't think "Handle fixed point conversion" is a descriptive enough title.

I am going to resolve FIXME in followup patches.

I assume by this you mean to move forward with adding floating-fixed conversions? I have an unsubmitted patch for APFixedPoint which implements the semantics of the conversions, but I'm currently waiting on D85312, D85314, and more recently I found D54749 which is probably a prerequisite for the IR codegen.

Yes,
I will add a function for fixed point to floating point conversion, (Example: handleFixedPointToFloatConversion()), similar to handleIntToFloatConversion() (SemaExpr.cpp)

Alright. There's a lot more that needs to be done to get the conversions to work, though. I would recommend having a look at some of the earlier patches, like D56900. The approach for codegen will probably be different, though. There's also the patches I mentioned earlier for moving all of the codegen+APFixedPoint code to LLVM, so things may move around in the future.

I can upload my partial APFixedPoint patch as a reference of the semantics of the conversions.

[gousemoodhin]

In D81904#2207399, @ebevhan wrote:

In D81904#2207335, @gousemoodhin wrote:

In D81904#2207262, @ebevhan wrote:

I don't think "Handle fixed point conversion" is a descriptive enough title.

I am going to resolve FIXME in followup patches.

I assume by this you mean to move forward with adding floating-fixed conversions? I have an unsubmitted patch for APFixedPoint which implements the semantics of the conversions, but I'm currently waiting on D85312, D85314, and more recently I found D54749 which is probably a prerequisite for the IR codegen.

Yes,
I will add a function for fixed point to floating point conversion, (Example: handleFixedPointToFloatConversion()), similar to handleIntToFloatConversion() (SemaExpr.cpp)

Alright. There's a lot more that needs to be done to get the conversions to work, though. I would recommend having a look at some of the earlier patches, like D56900. The approach for codegen will probably be different, though. There's also the patches I mentioned earlier for moving all of the codegen+APFixedPoint code to LLVM, so things may move around in the future.

I can upload my partial APFixedPoint patch as a reference of the semantics of the conversions.

Thanks for inputs,
I will follow the patches shared by you.

[gousemoodhin]

I don't have commit access. Please use these details to commit.
My Github username: gousemoodhin
My GitHub email id: [e-mail]

clang/lib/Sema/SemaExpr.cpp

QualType Sema::UsualArithmeticConversions(ExprResult &LHS, ExprResult &RHS,
 
   // At this point, we have two different arithmetic types.
 
   // Diagnose attempts to convert between __float128 and long double where
   // such conversions currently can't be handled.
   if (unsupportedTypeConversion(*this, LHSType, RHSType))
     return QualType();
 
+  // Conversion between the fixed point and floating point is unspecified.
+  // Reference N1169 4.1.4 (Type conversion, usual arithmetic conversions).
+  if ((LHSType->isFixedPointType() && RHSType->isRealFloatingType()) ||
+      (LHSType->isRealFloatingType() && RHSType->isFixedPointType()))
+    return QualType();
+
   // Handle complex types first (C99 6.3.1.8p1).
   if (LHSType->isComplexType() || RHSType->isComplexType())
     return handleComplexFloatConversion(*this, LHS, RHS, LHSType, RHSType,
                                         ACK == ACK_CompAssign);
 
   // Now handle "real" floating types (i.e. float, double, long double).
   if (LHSType->isRealFloatingType() || RHSType->isRealFloatingType())
     return handleFloatConversion(*this, LHS, RHS, LHSType, RHSType,