Add a static analysis job to prevent assertions with side effects #1881
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
User |
|
User |
newren
force-pushed
the
assertion-side-effects
branch
from
4c66803 to
20c763f
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
User |
|
On the Git mailing list, Taylor Blau wrote (reply to this): On Sun, Mar 16, 2025 at 06:41:59AM +0000, Elijah Newren via GitGitGadget wrote:
> Elijah Newren (3):
> git-compat-util: introduce BUG_IF_NOT() macro
> ci: add build checking for side-effects in assert() calls
> treewide: replace assert() with BUG_IF_NOT() in special cases
Nice, this version looks great to me. I left a couple of notes
throughout, but they range from "could be done later" to idle
commentary. Thanks for working on this, and I'm sorry to have sent you
down such a rabbit hole ;-).
Reviewed-by: Taylor Blau <[email protected]>
Thanks,
Taylor |
Create a ASSERT() macro which is similar to assert(), but will not be compiled out when NDEBUG is defined, and is thus safe to use even if its argument has side-effects. We will use this new macro in a subsequent commit to convert a few existing assert() invocations to ASSERT(). In particular, we'll convert the handful of invocations which cannot be proven to be free of side effects with a simple compiler/linker hack. Signed-off-by: Elijah Newren <[email protected]>
It is a big no-no to have side-effects in an assertion, because if the assert() is compiled out, you don't get that side-effect, leading to the code behaving differently. That can be a large headache to debug. We have roughly 566 assert() calls in our codebase (my grep might have picked up things that aren't actually assert() calls, but most appeared to be). All but 9 of them can be determined by gcc to be free of side effects with a clever redefine of assert() provided by Bruno De Fraine (from https://stackoverflow.com/questions/10593492/catching-assert-with-side-effects), who upon request has graciously placed his two-liner into the public domain without warranty of any kind. The current 9 assert() calls flagged by this clever redefinition of assert() appear to me to be free of side effects as well, but are too complicated for a compiler/linker to figure that since each assertion involves some kind of function call. Add a CI job which will find and report these possibly problematic assertions, and have the job suggest to the user that they replace these with ASSERT() calls. Example output from running: ``` ERROR: The compiler could not verify the following assert() calls are free of side-effects. Please replace with ASSERT() calls. /home/newren/floss/git/diffcore-rename.c:1409 assert(!dir_rename_count || strmap_empty(dir_rename_count)); /home/newren/floss/git/merge-ort.c:1645 assert(renames->deferred[side].trivial_merges_okay && !strset_contains(&renames->deferred[side].target_dirs, path)); /home/newren/floss/git/merge-ort.c:794 assert(omittable_hint == (!starts_with(type_short_descriptions[type], "CONFLICT") && !starts_with(type_short_descriptions[type], "ERROR")) || type == CONFLICT_DIR_RENAME_SUGGESTED); /home/newren/floss/git/merge-recursive.c:1200 assert(!merge_remote_util(commit)); /home/newren/floss/git/object-file.c:2709 assert(would_convert_to_git_filter_fd(istate, path)); /home/newren/floss/git/parallel-checkout.c:280 assert(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca)); /home/newren/floss/git/scalar.c:244 assert(have_fsmonitor_support()); /home/newren/floss/git/scalar.c:254 assert(have_fsmonitor_support()); /home/newren/floss/git/sequencer.c:4968 assert(!(opts->signoff || opts->no_commit || opts->record_origin || should_edit(opts) || opts->committer_date_is_author_date || opts->ignore_date)); ``` Note that if there are possibly problematic assertions, not necessarily all of them will be shown in a single run, because the compiler errors may include something like "ld: ... more undefined references to `not_supposed_to_survive' follow" instead of listing each individually. But in such cases, once you clean up a few that are shown in your first run, subsequent runs will show (some of) the ones that remain, allowing you to iteratively remove them all. Helped-by: Bruno De Fraine <[email protected]> Signed-off-by: Elijah Newren <[email protected]>
When the compiler/linker cannot verify that an assert() invocation is free of side effects for us (e.g. because the assertion includes some kind of function call), replace the use of assert() with ASSERT(). Signed-off-by: Elijah Newren <[email protected]>
newren
force-pushed
the
assertion-side-effects
branch
from
20c763f to
82b7344
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Taylor Blau wrote (reply to this): On Wed, Mar 19, 2025 at 04:22:55PM +0000, Elijah Newren via GitGitGadget wrote:
> Changes since v1:
>
> * Tweaked commit message for patch 2 Changes since v2:
> * Rename BUT_IF_NOT() -> ASSERT(). Didn't have a strong opinion on the set
> of alternatives Junio gave, so went with Taylor's small preference. If
> anyone has a strong preference here, I can pick a different alternative.
> * Fixed shell style issues (indentation, multi-line pipes, multiple lines
> with stderr redirects) in patch 2
>
> Elijah Newren (3):
> git-compat-util: introduce ASSERT() macro
> ci: add build checking for side-effects in assert() calls
> treewide: replace assert() with ASSERT() in special cases
>
> Makefile | 4 ++++
> ci/check-unsafe-assertions.sh | 18 ++++++++++++++++++
> ci/run-static-analysis.sh | 2 ++
> diffcore-rename.c | 2 +-
> git-compat-util.h | 8 ++++++++
> merge-ort.c | 4 ++--
> merge-recursive.c | 2 +-
> object-file.c | 2 +-
> parallel-checkout.c | 2 +-
> scalar.c | 4 ++--
> sequencer.c | 2 +-
> 11 files changed, 41 insertions(+), 9 deletions(-)
> create mode 100755 ci/check-unsafe-assertions.sh
Thanks, this version LGTM.
Thanks,
Taylor |
|
This patch series was integrated into seen via git@d8e081e. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@2a069bb. |
|
This patch series was integrated into seen via git@d733f65. |
|
There was a status update in the "Cooking" section about the branch Ensure what we write in assert() does not have side effects, and introduce ASSERT() macro to mark those that cannot be mechanically checked for lack of side effects. Will merge to 'next'. source: <[email protected]> |
|
This patch series was integrated into seen via git@d9993e8. |
|
This patch series was integrated into seen via git@db1177d. |
|
This patch series was integrated into next via git@de89bb3. |
|
There was a status update in the "Cooking" section about the branch Ensure what we write in assert() does not have side effects, and introduce ASSERT() macro to mark those that cannot be mechanically checked for lack of side effects. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@94c7f5e. |
|
This patch series was integrated into seen via git@27a3607. |
|
This patch series was integrated into seen via git@b97b360. |
|
This patch series was integrated into master via git@b97b360. |
|
This patch series was integrated into next via git@b97b360. |
|
Closed via b97b360. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have several hundred assert() invocations in our code base. Some have suggested that we should add a recommendation in our CodingGuidelines to avoid their use, because there is a risk that someone might include something with a side-effect in their assertion, which can lead to a very difficult to debug problem. However, CodingGuidelines are going to be less effective at preventing that foot-gun than a CI job which can warn of assertions that possibly have side-effects. So, let's add a CI job instead.
While it is difficult to perfectly determine whether any expression has side effects, a simple compiler/linker hack can prove that all but 9 of our several hundred assert() calls are indeed free from them. While I believe the remaining 9 are also free of side effects, it's easier to just convert those 9 to a new macro (which will not be compiled out when NDEBUG is defined), and instruct any future assertion writers to likewise switch to that alternative macro if they have a slightly more involved assert() invocation.
See https://github.com/newren/git/actions/runs/13845548634/job/38743076293#step:4:1938 for an example of it running in CI and reporting possibly problematic assertions (sample output also included in the commit message of the middle commit in this series if you don't have access to view the link; I'm not sure what the rules on that are).
Changes since v1:
Changes since v2:
cc: "brian m. carlson" [email protected]
cc: Elijah Newren [email protected]
cc: Taylor Blau [email protected]