This is an archive of the discontinued LLVM Phabricator instance.

[Diagnostics] Check integer to floating point number implicit conversions
AbandonedPublic

Authored by xbolva00 on Oct 3 2018, 10:54 AM.

Diff Detail

Event Timeline

xbolva00 created this revision.Oct 3 2018, 10:54 AM
xbolva00 updated this revision to Diff 168137.Oct 3 2018, 10:57 AM
xbolva00 updated this revision to Diff 168140.Oct 3 2018, 11:02 AM
  • Added tests
xbolva00 edited the summary of this revision. (Show Details)Oct 3 2018, 11:03 AM
lebedev.ri added inline comments.
test/Sema/ext_vector_casts.c
121

How is floating-point precision is being lost if the cast is from an integer?
I'm guessing you want to add a new diag with a more appropriate description.

xbolva00 updated this revision to Diff 168141.Oct 3 2018, 11:16 AM
xbolva00 added inline comments.
test/Sema/ext_vector_casts.c
121

yeah :/ so maybe integer precision?

lebedev.ri added inline comments.Oct 3 2018, 11:19 AM
test/Sema/ext_vector_casts.c
121

It's still rather a bit too vague i'd say.
It should describe what you actually checked.

xbolva00 updated this revision to Diff 168150.Oct 3 2018, 11:47 AM
  • better warning
erichkeane added inline comments.Oct 3 2018, 12:31 PM
lib/Sema/SemaChecking.cpp
110

Hmm.... I don't terribly understand how this function works. Also, comment above needs to end in a period.

Can you elaborate further as to how this works? Those are 3 pretty suspect looking magic numbers...

10862

Since there is only 1, and fltSemantics is really small (2 shorts + 2 uints), making this a 'by copy' is likely a better solution than a pointer.

10867

This else case ends up needing to be handled above, but why not also handle the other floating types?

10873

It seems like this should be checked way before we convert it to an integer or do the other float-semantics disambiguation.

10877

Is this the rounding mode that we use? Also, you might need to check this against some sort of current state for rounding mode. I know that there is an effort to do FENV_ACCESS, which this might change, right?

10883

I wonder if this call (finding the precision, 'adjusting' it, then writing to a smallstring might be a better thing to pull into its own function rather than AdjustPrecision.

scanon added a subscriber: scanon.Oct 3 2018, 12:40 PM
scanon added inline comments.
lib/Sema/SemaChecking.cpp
110

It's attempting to compute the number of good base-10 digits (59/196 ~= log2(10)). We should really just make APFloat print the shortest round-trippable digit sequence instead. Yes, this is tricky to implement, but we don't need to implement it. There are two recent high-quality implementations available, which are both significantly faster than previous algorithms: Ryu and Swift's (https://github.com/apple/swift/blob/master/stdlib/public/runtime/SwiftDtoa.cpp). Swift's has the virtue of already being used in LLVM-family languages and having a tidy single-file implementation, but either would be perfectly usable, I think.

Neither supports float128 yet, but we could simply drop them in for float, double, and float80.

xbolva00 added inline comments.Oct 3 2018, 1:44 PM
lib/Sema/SemaChecking.cpp
10862

llvm::fltSemantics has incomplete type and cannot be defined :/ Quite strange error

xbolva00 added inline comments.Oct 3 2018, 1:47 PM
lib/Sema/SemaChecking.cpp
10883

I go thru LLVM code and I see we use rmNearestTiesToEven. So I am gonna change it.

craig.topper added inline comments.
lib/Sema/SemaChecking.cpp
110

Function names should start with lower case I think?

111

Can we use divideCeil from llvm/Support/MathExtras.h?

10881

Variable name should start with uppercase

xbolva00 added inline comments.Oct 3 2018, 2:05 PM
lib/Sema/SemaChecking.cpp
110

float80 has precision = 64 so we can put MAX value of unsigned long long into it with no issues, or?

xbolva00 added inline comments.Oct 3 2018, 2:08 PM
lib/Sema/SemaChecking.cpp
110

Other static functions here use upper case, e.g. "PrettyPrintInRange", so I follow it :)

xbolva00 updated this revision to Diff 168171.Oct 3 2018, 2:18 PM
  • Addressed some comments
xbolva00 marked 4 inline comments as done.Oct 3 2018, 2:19 PM
xbolva00 updated this revision to Diff 168173.Oct 3 2018, 2:26 PM
  • Check also int128_t to float
lib/Sema/SemaChecking.cpp
110

float80 has precision = 64 so we can put MAX value of unsigned long long into it with no issues, or?

ah, int128_t

xbolva00 updated this revision to Diff 168176.Oct 3 2018, 2:33 PM
  • Support for Float16
rsmith added inline comments.Oct 3 2018, 5:09 PM
lib/Sema/SemaChecking.cpp
110

LLVM will need to carry an implementation of the "shortest round-trippable digit sequence" soon regardless of what we do here, because it's part of the C++17 standard library. We should aim for an implementation that can be shared by APFloat and libc++.

xbolva00 added inline comments.Oct 3 2018, 9:42 PM
lib/Sema/SemaChecking.cpp
110

You are right. But anyway, it should not block this patch, no? I can put TODO comment there.

This comment was removed by xbolva00.

Any comments? Waiting for approval here to move focus on some other patches :)

Probably should have a test for something like float x = (__uint128_t)-1;, to make sure we print something reasonable.

lib/Sema/SemaChecking.cpp
10874

ASTContext::getFloatTypeSemantics. (Your explicit computation here is both redundant and wrong.)

xbolva00 added inline comments.Oct 11 2018, 12:26 AM
lib/Sema/SemaChecking.cpp
10874

Great tip! thanks. I overlooked it :/

xbolva00 updated this revision to Diff 169162.Oct 11 2018, 12:43 AM
  • Addressed comments
efriedma added inline comments.Oct 11 2018, 12:48 PM
lib/Sema/SemaChecking.cpp
10878–10879

It looks like this is reusing an existing diagnostic; what warning flag is this controlled by? It it possible we need a separate flag for this warning?

xbolva00 added inline comments.Oct 11 2018, 12:52 PM
lib/Sema/SemaChecking.cpp
10878–10879

This is in LiteralConversion.

As noted, this in under LiteralConversion (-Wconversion). GCC has it too under -Wconversion, so I think it is fine as is, or?
@rsmith

As noted, this in under LiteralConversion (-Wconversion). GCC has it too under -Wconversion, so I think it is fine as is, or?
@rsmith

It's not so much about "which flag group do i need to enable to get the warning";
it's about "which flag do i need to disable to silence it", and "how fine-grained that flag is, how many warnings will get disabled".
The flags should be pretty fine-grained.

xbolva00 added a comment.EditedOct 12 2018, 12:16 AM

So any suggestions? LiteralPrecision? ..

xbolva00 updated this revision to Diff 169567.Oct 13 2018, 8:59 AM
  • New diagnostic group

Looks like the latest update is missing a test file?

xbolva00 updated this revision to Diff 169881.Oct 16 2018, 12:58 PM
  • Added missing test file
nhaehnle removed a subscriber: nhaehnle.Oct 17 2018, 3:18 AM

Is PrecisionConversion acceptable? If not, any suggestions for name?

Ping

Anything else to be done here? I would like to finish this patch to work on other things.

maybe @aaron.ballman wants to take a look too

Since I don't think we have any false positives here, maybe it would be useful to add it to -Wall or -Wextra?

xbolva00 updated this revision to Diff 172509.Nov 3 2018, 4:17 PM

Since no futher comments, i think it is ok.
After day or two I will commit it.

xbolva00 accepted this revision.Nov 14 2018, 6:09 AM
This revision is now accepted and ready to land.Nov 14 2018, 6:09 AM
aaron.ballman added inline comments.Nov 14 2018, 6:20 AM
lib/Sema/SemaChecking.cpp
113–115

Parameter names should be in PascalCase, not camelCase.

10864–10865

It seems wrong to early return here -- that means none of the later checks are run on system macros, but we've also not diagnosed anything as being wrong with the user's code yet.

10882

This doesn't seem like something we need to early return for?

This revision was automatically updated to reflect the committed changes.
xbolva00 reopened this revision.Nov 14 2018, 6:30 AM
This revision is now accepted and ready to land.Nov 14 2018, 6:30 AM
xbolva00 added a comment.EditedNov 14 2018, 6:31 AM

Reverted.
Ok, I will address your comments soon.

Reverted.
Ok, I will address your comments soon.

Thanks! I'm sorry for the delayed reviews -- there were wg21 meetings last week, which several of the reviewers were attending.

Are you sure check-clang passes with this patch?
It looks like some bots weren't happy before the revert.
(Also, is this warning enabled when building llvm? If yes, you might want to double-check stage2 build.)

xbolva00 updated this revision to Diff 174505.Nov 17 2018, 5:29 AM

Addressed comments, fixed tests

It looks like you removed a considerable amount of testing coverage; why?

lib/Sema/SemaChecking.cpp
10864–10865

This changes the behavior of your patch -- I think it made sense to not trigger this diagnostic in a system macro. I was suggesting that you replace the early return with braces and flip the logic around so that you only do the diagnostic work if you're not in a system macro.

xbolva00 added a comment.EditedNov 18 2018, 9:12 AM

I removed _Float16 related tests since some bots may fail with it, i dont know much about this custom type.
Anyway, It is fine on linux x86 64. I will restore them.

System macro - I will fix it.

xbolva00 updated this revision to Diff 175777.Nov 28 2018, 2:57 PM
  • Updated. Addressed review comments.
xbolva00 requested review of this revision.Nov 28 2018, 2:58 PM
aaron.ballman added inline comments.Nov 29 2018, 4:59 AM
test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp
136 ↗(On Diff #175777)

I don't think we want the warning triggered in either of these cases -- they already have an error diagnostic on the same line for the same issue.

test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
124 ↗(On Diff #175777)

I think we don't want to duplicate the warnings here, either.

xbolva00 marked an inline comment as done.Dec 16 2018, 1:24 PM
xbolva00 added inline comments.
test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp
136 ↗(On Diff #175777)

Any recommended way how it should be handled?

xbolva00 added a comment.EditedFeb 27 2019, 11:12 AM

@aaron.ballman does it make sense to warn for this case only in C/pre-C++11 mode? Since this case in C++11/14/17 is handled by -Wnarrowing, as we see from tests.

Or any other solution?

@rsmith

@aaron.ballman does it make sense to warn for this case only in C/pre-C++11 mode? Since this case in C++11/14/17 is handled by -Wnarrowing, as we see from tests.

I don't think so, because code like this still needs the diagnostic even in C++11 mode, but there's no -Wnarrowing warning for it: float a1 = (1ULL << 31) + 1;

test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp
136 ↗(On Diff #175777)

Why is it triggering twice in the first place? Template instantiation, maybe?

xbolva00 abandoned this revision.Apr 29 2019, 2:08 PM