Skip to content

Mimic GCC/Clang simplification behaviour when type checking ?: #7959

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

tautschnig
Copy link
Collaborator

Neither GCC nor Clang simplify expressions involving symbols even when the result would always be 0, which our simplifier figure out.

Fixes: #7927

  • Each commit message has a non-empty body, explaining why the change was made.
  • n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • n/a My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

❌ Patch coverage is 52.94118% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.38%. Comparing base (3e5b027) to head (0846117).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/ansi-c/c_typecheck_expr.cpp 52.94% 40 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7959      +/-   ##
===========================================
- Coverage    80.39%   80.38%   -0.01%     
===========================================
  Files         1688     1688              
  Lines       207403   207435      +32     
  Branches        73       73              
===========================================
+ Hits        166749   166756       +7     
- Misses       40654    40679      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kroening
Copy link
Member

We will eventually need explicit constant folding logic in the C frontend, as opposed to relying on the simplifier. I expect that the latter will always be able to simplify more.

@kroening
Copy link
Member

This PR needs more contemplation. The PR changes the typing, but would appear to be about constant folding.
Maybe the presence of symbols doesn't change the typing, but does affect constant folding.

@tautschnig tautschnig force-pushed the features/gcc-clang-ite-simp branch from 506cd0d to 6f82cc7 Compare November 16, 2023 09:20
@tautschnig
Copy link
Collaborator Author

This PR needs more contemplation. The PR changes the typing, but would appear to be about constant folding. Maybe the presence of symbols doesn't change the typing, but does affect constant folding.

@kroening: Please take another look, I hope to have adjusted this in line with your feedback as it now uses (compile-time) constant folding rules.

@tautschnig tautschnig assigned tautschnig and unassigned kroening Nov 20, 2023
@tautschnig tautschnig force-pushed the features/gcc-clang-ite-simp branch from 6f82cc7 to 3421bfd Compare December 13, 2023 13:58
@tautschnig tautschnig assigned kroening and unassigned tautschnig Dec 13, 2023
_Static_assert(
sizeof(int) != sizeof(*(1 ? ((void *)(i ? 0ll : 0ll)) : (int *)1)), "");
_Static_assert(
sizeof(int) != sizeof(*(1 ? ((void *)(0 ? i : 0ll)) : (int *)1)), "");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These appear to assert that some part of the subexpression is a compile time constant. May I suggest to use __builtin_constant_p for that. The sizeof assertion is a distraction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This, I'm afraid, just uncovered another challenge: the above "true" cases aren't deemed null-pointer constants by the compilers (and, therefore, the type of the ?: is void* rather than int*). __builtin_constant_p, however, does return 1 for the very same expressions!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a commit that better exercises (and extends) an existing test to also document (and test) these differences.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhhh, sad

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth filing a bug report with those compilers.

@tautschnig tautschnig force-pushed the features/gcc-clang-ite-simp branch 2 times, most recently from 433de81 to 4076a22 Compare December 13, 2023 15:41
@kroening
Copy link
Member

Apologies that the scope of this PR is getting pushed larger, by me. Does it make sense to replace the minimal case split done for __builtin_constant_p by an invocation of is_compile_time_constantt(ns) plus some special cases for when they differ?

@tautschnig tautschnig self-assigned this Dec 13, 2023
@tautschnig
Copy link
Collaborator Author

Apologies that the scope of this PR is getting pushed larger, by me. Does it make sense to replace the minimal case split done for __builtin_constant_p by an invocation of is_compile_time_constantt(ns) plus some special cases for when they differ?

I'll take care of this!

@tautschnig tautschnig force-pushed the features/gcc-clang-ite-simp branch from 4076a22 to d39db8f Compare December 14, 2023 20:07
@tautschnig
Copy link
Collaborator Author

Apologies that the scope of this PR is getting pushed larger, by me. Does it make sense to replace the minimal case split done for __builtin_constant_p by an invocation of is_compile_time_constantt(ns) plus some special cases for when they differ?

I'll take care of this!

I believe this is now done.

@tautschnig tautschnig assigned tautschnig and unassigned tautschnig Dec 14, 2023
}
else
is_constant=tmp1.is_constant();
{
// try to produce constant
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_compile_time_constantt is too far off in the GCC/MSVC case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__builtin_constant_p doesn't exist with MSVC, so it'd be GCC only. For that, however, the simplifier appears to be much closer: GCC happily takes x - x, x * 0 as constants. But then we could still do what I now (in my latest push to this PR) ended up doing: produce a variant of is_compile_time_constantt for a specific compiler (clang_is_constant_foldedt).

@tautschnig tautschnig force-pushed the features/gcc-clang-ite-simp branch 3 times, most recently from 0c1b2f7 to 5d04507 Compare December 14, 2023 21:12
@tautschnig tautschnig removed their assignment Dec 14, 2023
@tautschnig tautschnig force-pushed the features/gcc-clang-ite-simp branch 2 times, most recently from f86884c to 2d5adef Compare December 14, 2023 23:11
@tautschnig tautschnig force-pushed the features/gcc-clang-ite-simp branch 2 times, most recently from 5772645 to f2ed870 Compare July 28, 2025 18:02
This is useful in various methods and should be available to all of
them.
Neither GCC nor Clang simplify expressions involving symbols even when
the result would always be 0, which our simplifier figure out.

Fixes: diffblue#7927
C11 Section 6.6 "Constant expressions" defines what are required to be
constant expressions, and permits implementations to pick additional
constant expressions.
We previously only compile-time tested what required a run-time check of
assertions, so move this to the "cbmc" regression test suite. Also,
extend it by behaviour that distinguishes it from actual C-standard
specified constant expressions. Finally, fix the behaviour for string
literals.
@tautschnig tautschnig force-pushed the features/gcc-clang-ite-simp branch from f2ed870 to 0846117 Compare July 29, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Goto-cc doesn't handle BUILD_BUG_ON_ZERO macro in Linux
2 participants