Page MenuHomePhabricator

[Sema] Adds the pointer-to-int-cast diagnostic
ClosedPublic

Authored by Mordante on Jan 5 2020, 11:41 AM.

Details

Summary

Converting a pointer to an integer whose result cannot represented in the integer type is undefined behavior is C and prohibited in C++. C++ already has a diagnostic when casting. This adds a diagnostic for C.

The diagnostic is not enabled by default due to the number of diagnostics it triggered while running the tests.

Since this diagnostic uses the range of the conversion it also modifies int-to-pointer-cast diagnostic to use a range.

Fixes PR8718: No warning on casting between pointer and non-pointer-sized int

Diff Detail

Event Timeline

Mordante created this revision.Jan 5 2020, 11:41 AM

The diagnostic is not enabled by default

But GCC enables it for C even without "-Wall or -Wextra". Clang should follow it..

Unit tests: pass. 61248 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

The diagnostic is not enabled by default

But GCC enables it for C even without "-Wall or -Wextra". Clang should follow it..

I'll have a look what the impact is. When I tested it about 20 tests failed.

Mordante planned changes to this revision.Jan 7 2020, 12:59 PM

While looking at the test failures I noticed the tests pointer_to_integral_type_conv in clang/test/Sema/MicrosoftExtensions.c were not tested. Will look into it later.

rjmccall added a subscriber: rjmccall.

It's not unusual for new warnings to require changes to other tests. I agree with enabling this by default.

Mordante updated this revision to Diff 242393.Tue, Feb 4, 11:51 AM
  • Enabled the warning by default
  • Added an Microsoft extension, the unit tests already expected this behaviour
rsmith accepted this revision.Tue, Feb 4, 12:06 PM

Looks fine.

Would it make sense to put the MS extension warning into the -Wpointer-to-int-cast group so that we can control this warning consistently across platforms? You could get that effect by introducing a new warning group (eg, "-Wmicrosoft-pointer-to-int-cast") containing just the ExtWarn warning, and putting that group in both MicrosoftCast and PointerToIntCast.

clang/test/Sema/cast.c
1

Don't specify the warning flag here, so that we have some test coverage that this is enabled by default.

This revision is now accepted and ready to land.Tue, Feb 4, 12:06 PM
Mordante marked 2 inline comments as done.Sun, Feb 16, 6:40 AM

Thanks for the review.

I'll have a look at your suggestion for a follow-up patch.

clang/test/Sema/cast.c
1

Good catch! This was needed before the diagnostic was enabled by default.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.

This revision caused issue for the MSVC build bot. I created a followup patch D74694.

There appear to a be semantic difference between GCC and clang with the current version of this patch which results in a lot of additional warnings in the Linux kernel: https://godbolt.org/z/eHFJd8