Page MenuHomePhabricator

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

Authored by gousemoodhin on Jun 15 2020, 8:10 PM.

Details

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.

Diff Detail

Event Timeline

gousemoodhin created this revision.Jun 15 2020, 8:10 PM
gousemoodhin retitled this revision from clang: Handle Fixed point unspecified case to [clang] Handle Fixed point Conversion.
gousemoodhin edited the summary of this revision. (Show Details)

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 retitled this revision from [clang] Handle Fixed point Conversion to [clang] Handle fixed point conversion.
gousemoodhin edited the summary of this revision. (Show Details)
gousemoodhin added inline comments.Aug 2 2020, 9:44 PM
clang/lib/Sema/SemaExpr.cpp
1523

Reworked on comments, uploaded a new patch.

1527

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

gousemoodhin edited the summary of this revision. (Show Details)Aug 2 2020, 9:45 PM
gousemoodhin marked an inline comment as done.Aug 2 2020, 9:49 PM
gousemoodhin marked an inline comment as done.

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

gousemoodhin edited the summary of this revision. (Show Details)

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

Done.

ebevhan added inline comments.Aug 7 2020, 5:18 AM
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 marked an inline comment as done.Aug 7 2020, 9:15 AM
gousemoodhin added inline comments.
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 marked an inline comment as done.Aug 7 2020, 9:16 AM
gousemoodhin added inline comments.
clang/test/Frontend/fixed_point_errors.c
257

Done

gousemoodhin edited the summary of this revision. (Show Details)
gousemoodhin edited the summary of this revision. (Show Details)
gousemoodhin edited the summary of this revision. (Show Details)

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 edited the summary of this revision. (Show Details)Aug 10 2020, 8:59 AM
This comment was removed by gousemoodhin.

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.

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)

gousemoodhin retitled this revision from [clang] Handle fixed point conversion to [clang] Do not crash for unsupported fixed point to floating point conversion.Aug 10 2020, 10:14 AM
ebevhan accepted this revision.Aug 10 2020, 10:24 AM

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.

This revision is now accepted and ready to land.Aug 10 2020, 10:24 AM

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.

I don't have commit access. Please use these details to commit.
My Github username: gousemoodhin
My GitHub email id: nadafgouse5@gmail.com

Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2020, 8:30 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript