`A5-0-1`: False positive related to overeager alias analysis

Open
#1,047 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
35/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Stale
Tech stack
cpp
Domain
devtools

Research direction

Start with the SideEffects library and the affected A5-0-1 analysis, then inspect the references to localFlow and AliasParameter. Compare the provided examples with the handling of expression side-effects and function-call side-effects, including the RULE-13-2 case. Done means alias analysis no longer reports false unsequenced side-effects while still detecting genuine effects through function parameters.

Written by the indexing model from the issue text.

Description

false positive/false negative
Affected rules
  • A5-0-1
  • Likely others such as RULE-13-2 under more atypical conditions
Description

We can produce false positives related to unsequenced side effects from overeager alias analysis.

The SideEffects library attempts to capture side effects across function boundaries:

void f(int *i) {
  (*i)++; // side-effect (*i)++
}

void g() {
  int i;
  f(&i); // side-effect: (*i)++
}

Detecting the latter case requires flow analysis. Currently, we perform flow analysis on all address-of operators, not just to parameters or through "other" functions. This leads to weird behavior:

void h() {
  int i;
  int j = &i;
  i = 1;
  j; // according to our analysis, this has a side-effect: i = 1
  j + j; // reported as unsequenced side-effects
}

Our side effect analysis should be split into two distinct phases: side-effects within expressions, and side-effects from function calls.

It looks like we mostly do this, but incorrectly.

Side-effects within expressions

Should only use flow analysis to find aliases for cases such as:

void f() {
  int i = 0;
  int j = &i;

  int x = i-- + (*j++); // unsequenced effects due to aliasing
}

That is, we can detect that *j is an alias of i and effectively consider the side-effects of i-- + i++. This is different than declaring *j has side-effects.

This is not currently handled correctly, as we inaccurately assign side-effects via localFlow.

Side-effects from function calls
void f(int *x) {
  (*x)++;
}

We should recursively find parameters that become the subjects of side-effects. We should trace parameter x to the effect on x. Now f(x) can be discovered in the above phase as an effect on x -- without flow analysis.

We implement this as AliasParameter, but we don't find local flow from a parameter to its effect, we just look for effects on param.getAnAccess()

Example

Real code example in pandas

      while (*step) {
         stbtt__active_edge * z = *step;
         if (z->ey <= scan_y_top) {
            *step = z->next; // delete from list
            STBTT_assert(z->direction);
            z->direction = 0;
            stbtt__hheap_free(&hh, z);
         } else {
            step = &((*step)->next); // advance through list
         }
      }
Dominant language
CodeQL
Stars
227
Forks
82
Avg merge
6d 7h
Merged PRs (30d)
9

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from github/codeql-coding-standards

All issues in github/codeql-coding-standards

Similar issues

More DevTools issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.