Page MenuHomePhabricator

[analyzer] ConversionChecker: handle floating point
ClosedPublic

Authored by donat.nagy on Oct 1 2018, 9:53 AM.

Details

Summary

Extend the alpha.core.Conversion checker to handle implicit converions
where a too large integer value is converted to a floating point type. Each
floating point type has a range where it can exactly represent all integers; we
emit a warning when the integer value is above this range. Although it is
possible to exactly represent some integers which are outside of this range
(those that are divisible by a large enough power of 2); we still report cast
involving those, because their usage may lead to bugs. (For example, if 1<<24
is stored in a float variable x, then x==x+1 holds.)

Diff Detail

Repository
rC Clang

Event Timeline

donat.nagy created this revision.Oct 1 2018, 9:53 AM
Szelethus added inline comments.Oct 1 2018, 10:29 AM
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
176

How about AC.getSizeType(AC.UnsignedLongLongTy))?

test/Analysis/conversion.c
190

Need formatting.

193

This too -- how about running clang-format on this file?

Szelethus added inline comments.
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
176

I'm actually not too sure about this. @whisperity?

NoQ added inline comments.Oct 1 2018, 3:29 PM
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
176

Yeah, i suspect it's a host machine check (to prevent our own overflow on line 189) rather than a target machine check.

donat.nagy retitled this revision from [analysis] ConversionChecker: handle floating point to [analyzer] ConversionChecker: handle floating point.Oct 2 2018, 2:49 AM
Szelethus added inline comments.Oct 2 2018, 3:22 AM
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
176

Oh right, thanks for clarifying that.
I looked it up, and the number of bits in a char can be acquired from CHAR_BITS, maybe use that instead of hard coding 8? https://en.cppreference.com/w/c/types/limits

donat.nagy updated this revision to Diff 167929.Oct 2 2018, 5:51 AM

Fix formatting in tests; add a comment.

donat.nagy marked 6 inline comments as done.Oct 2 2018, 5:54 AM
donat.nagy added inline comments.
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
176

Yes, this is intended to be a host machine check (corresponding to the W >= 64U in the old version). I will add a comment to clarify this.

xazax.hun added inline comments.Oct 18 2018, 2:09 AM
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
163

A link to the source of these number would be useful. How are these calculated. Also, as far as I know the current C++ standard does not require anything about how floating points are represented in an implementation. So it would be great to somehow refer to the representation used by clang rather than hardcoding these values. (Note that I am perfectly fine with warning for implementation defined behavior as the original version also warn for such in case of integers.)

171

Comment should be full sentences with a full stop at the end.

172

Do not add redundant braces when we only guard one line. It is perfectly ok when the one statements spans over multiple lines (maybe due to comments).

donat.nagy added inline comments.Oct 18 2018, 8:40 AM
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
163

I took these magic numbers from the IEEE 754 standard; I completely agree that their introduction is far from being elegant.

Unfortunately it seems that referring to the representation used by clang seems to be somewhat difficult, see e.g. this old stackoverflow answer. In the Z3 solver a similar problem was solved by defining a static function (getFloatSemantics()) which uses a switch to translate the bit width of a floating point type into an llvm::fltSemantics value (which contains the precision value as a field).

I could imagine three solutions:

  • reimplementing the logic getFloatSemantics,
  • moving getFloatSemantics to some utility library and using it from there,
  • keeping the current code, with comments describing my assumptions and referencing the IEEE standard.

Which of these is the best?

Note: According to the documentation the floating point types supported by the LLVM IR are just float, double and some high precision extension types (which are handled by the default: branch of my code). Unfortunately, I do not know what happens to the _Float16 half-width float type.

Give a reference for the significand size values of the IEEE 754 floating point types; cleanup comments and formatting.

donat.nagy marked 2 inline comments as done.Oct 19 2018, 2:29 AM
donat.nagy added inline comments.
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
163

I updated the patch with a comment describing my assumptions, but I will implement a different solution if that would be better.

NoQ added inline comments.Oct 19 2018, 2:45 PM
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
164

Continuing the float semantics discussion on the new revision - Did you consider llvm::APFloat? (http://llvm.org/doxygen/classllvm_1_1APFloat.html) This looks like something that you're trying to re-implement.

donat.nagy added inline comments.Oct 24 2018, 4:56 AM
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
164

This switch statement essentially fulfills two functions: it maps QualTypes to standardized IEEE floating point types and it immediately maps the standardized IEEE types to their precision values.

The difficulty is that I don't think that the first map is available as a public function in the clang libraries. This means that although a switch over the bit width of the floating point type is certainly inelegant, I cannot avoid it.

The second map is available in the APFloat library (via the llvm::fltSemantics constants, which describe the standard IEEE types). However, this map is extremely stable (e.g. I don't think that the binary structure of the IEEE double type will ever change), so I think that re-implementing it is justified by the fact that it yields shorter and cleaner code. However, as I had noted previously, I am open to using the llvm::fltSemantics constants to implement this mapping.

As an additional remark, my current code supports the _Float16 type, which is currently not supported by the APFloat/fltSemantics library. If we decide to use the llvm::fltSemantics constants, then we must either extend the APFloat library or apply some workarounds for this case.

NoQ added inline comments.Oct 25 2018, 1:52 PM
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
164

The difficulty is that I don't think that the first map is available as a public function in the clang libraries. This means that although a switch over the bit width of the floating point type is certainly inelegant, I cannot avoid it.

fltSemantics are not just enum constants, they contain a lot of fancy data:

static const fltSemantics semIEEEhalf = {15, -14, 11, 16};
static const fltSemantics semIEEEsingle = {127, -126, 24, 32};
static const fltSemantics semIEEEdouble = {1023, -1022, 53, 64};
static const fltSemantics semIEEEquad = {16383, -16382, 113, 128};
static const fltSemantics semX87DoubleExtended = {16383, -16382, 64, 80};

Use APFloat to determine precision of floating point type

Additionally, fix a typo in the tests.

xazax.hun accepted this revision.Oct 30 2018, 5:27 AM

LGTM!
I only wonder if this should be on by default or guarded by a config option. I do not have strong feelings about any of the options though.

This revision is now accepted and ready to land.Oct 30 2018, 5:27 AM
donat.nagy marked 7 inline comments as done.Oct 30 2018, 5:45 AM

I found a solution for determining the precision value of a floating point type. Is this acceptable?

lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
164

I knew about the data members of fltSemantics, but I did not see a way to use them elegantly (although fltSemantics has sizeInBits as a member, the possible fltSemantics values are not collected into one data structure, so I could not look up the precision field).

However, as I examined the APFloat.h source again, I found the function llvm::APFloat::getAllOnesValue which allows me to map bit width to semantics. While this is a somewhat indirect solution, it avoids using a switch.

The `missing _Float16' problem mentioned in my previous comments was my mistake, somehow I managed to overlook semIEEEhalf.

NoQ accepted this revision.Nov 1 2018, 6:40 PM

The new addition makes perfect sense, please feel free to commit :) Eventually i guess we'll need to declare where is this checker going and what specific lint rule is this checker designed to check, document this rule, and move it to optin once it has no false positives with respect to that rule and warning messages are improved. I suspect that it would need trackNullOrUndefValue() aka trackExpressionValue() and the currently discussed constraint tracking so that to explain why does the checker thinks that the input value of the cast is constrained to be losing precision on the current path.

I only wonder if this should be on by default or guarded by a config option.

Off-by-default flags mostly make sense when the newly added feature is somehow more opt-in than the rest of the checker, and for now i don't see any indication of that. If it turns out that this part of the checker has its own specific problems, i guess we can put it behind a flag, otherwise kinda why bother?

lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
158–162

Hmm, so the remaining problem is how to extract float semantics from a float QualType? Would ASTContext::getFloatTypeSemantics(DestType) make sense?

test/Analysis/conversion.c
193

Tests are often unformatted and we usually don't care. Additionally, they are almost always violating LLVM's camelCase vs. under_score conventions.

donat.nagy updated this revision to Diff 172922.Nov 7 2018, 4:22 AM
donat.nagy marked an inline comment as done.

Use ASTContext::getFloatTypeSemantics()

donat.nagy marked an inline comment as done.Nov 7 2018, 4:23 AM

Could someone with commit rights commit this patch (if it is acceptable)? I don't have commit rights myself.

lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
158–162

Thank you, that is the method I was looking for!

Szelethus added inline comments.Nov 7 2018, 5:18 AM
test/Analysis/conversion.c
193

Hmm, okay :)

Could someone with commit rights commit this patch (if it is acceptable)? I don't have commit rights myself.

I'll do the honors. Thanks!

This revision was automatically updated to reflect the committed changes.