[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

1523

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.

1527

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

1523

Reworked on comments, uploaded a new patch.

1527

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

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

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

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

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

 /// Handle arithmethic conversion with floating point types.  Helper
 /// function of UsualArithmeticConversions()
 static QualType handleFloatConversion(Sema &S, ExprResult &LHS,
                                       ExprResult &RHS, QualType LHSType,
                                       QualType RHSType, bool IsCompAssign) {
   bool LHSFloat = LHSType->isRealFloatingType();
   bool RHSFloat = RHSType->isRealFloatingType();
 
+  // FIXME: Implement floating to fixed point conversion.(Bug 46268)
+  // Reference N1169 4.1.4 (Type conversion, usual arithmetic conversions).
+  if ((LHSType->isFixedPointType() && RHSFloat) ||
+      (LHSFloat && RHSType->isFixedPointType()))
+    return QualType();
   // If we have two real floating types, convert the smaller operand
   // to the bigger result.
   if (LHSFloat && RHSFloat) {
     int order = S.Context.getFloatingTypeOrder(LHSType, RHSType);
     if (order > 0) {
       RHS = S.ImpCastExprToType(RHS.get(), LHSType, CK_FloatingCast);
       return LHSType;
     }
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();
 
   // 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,
                                  ACK == ACK_CompAssign);
 
   // Handle GCC complex int extension.
   if (LHSType->isComplexIntegerType() || RHSType->isComplexIntegerType())
     return handleComplexIntConversion(*this, LHS, RHS, LHSType, RHSType,

clang/test/Frontend/fixed_point_errors.c

 unsigned u_const = -2.5hk;                // expected-warning{{implicit conversion from -2.5 cannot fit within the range of values for 'unsigned int'}}
 char c_const = 256.0uk;                   // expected-warning{{implicit conversion from 256.0 cannot fit within the range of values for 'char'}}
 short _Accum sa_const5 = 256;             // expected-warning{{implicit conversion from 256 cannot fit within the range of values for 'short _Accum'}}
 unsigned short _Accum usa_const2 = -2;    // expected-warning{{implicit conversion from -2 cannot fit within the range of values for 'unsigned short _Accum'}}
 
 short _Accum add_ovf1 = 255.0hk + 20.0hk;                     // expected-warning {{overflow in expression; result is -237.0 with type 'short _Accum'}}
 short _Accum add_ovf2 = 10 + 0.5hr;                           // expected-warning {{overflow in expression; result is 0.5 with type 'short _Fract'}}
 short _Accum sub_ovf1 = 16.0uhk - 32.0uhk;                    // expected-warning {{overflow in expression; result is 240.0 with type 'unsigned short _Accum'}}
 short _Accum sub_ovf2 = -255.0hk - 20;                        // expected-warning {{overflow in expression; result is 237.0 with type 'short _Accum'}}
 short _Accum mul_ovf1 = 200.0uhk * 10.0uhk;                   // expected-warning {{overflow in expression; result is 208.0 with type 'unsigned short _Accum'}}
 short _Accum mul_ovf2 = (-0.5hr - 0.5hr) * (-0.5hr - 0.5hr);  // expected-warning {{overflow in expression; result is -1.0 with type 'short _Fract'}}
 short _Accum div_ovf1 = 255.0hk / 0.5hk;                      // expected-warning {{overflow in expression; result is -2.0 with type 'short _Accum'}}
 
 short _Accum shl_ovf1 = 255.0hk << 8;           // expected-warning {{overflow in expression; result is -256.0 with type 'short _Accum'}}
 short _Fract shl_ovf2 = -0.25hr << 3;           // expected-warning {{overflow in expression; result is 0.0 with type 'short _Fract'}}
 unsigned short _Accum shl_ovf3 = 100.5uhk << 3; // expected-warning {{overflow in expression; result is 36.0 with type 'unsigned short _Accum'}}
 short _Fract shl_ovf4 = 0.25hr << 2;            // expected-warning {{overflow in expression; result is -1.0 with type 'short _Fract'}}

 short _Fract add_sat  = (_Sat short _Fract)0.5hr + 0.5hr;
 short _Accum sub_sat  = (_Sat short _Accum)-200.0hk - 80.0hk;
 short _Accum mul_sat  = (_Sat short _Accum)80.0hk * 10.0hk;
 short _Fract div_sat  = (_Sat short _Fract)0.9hr / 0.1hr;
 short _Accum shl_sat = (_Sat short _Accum)200.0hk << 5;
 
 // Division by zero
 short _Accum div_zero = 4.5k / 0.0lr;  // expected-error {{initializer element is not a compile-time constant}}
+
+void foo(void) {
+  _Accum x = 0.5k;
+  if (x == 0.5) {} // expected-error{{invalid operands to binary expression ('_Accum' and 'double')}}
+}