Major refactoring of array bounds code and some performance improvements

[Machiry]

• Implemented the technique similar to the locksmith, where the bounds of a variable is the intersection of the bounds of all array variables that flows into the variable.

• Added a flag to disable array bounds heuristics: -disable-arr-hu (By default, they are enabled but we can use this flag to disable them).

• We used same node for all ocurrences of constant numbers, for example, for the below code:

  int n = 8;
  int l = 8;
  

  We would construct our graph as: n <-> 8 <-> l, this wrongly leads us to believe that n is reachable (i.e., flows) into l. Which is not true.

  We changed the code to create a new node for every occurrence of constants and thereby avoid this.

• Added cache to breadth-first search, realized that we call BFS on the same node many times and it is better to cache this information (provided that the graph didn't change).

• This also fixes: Should pick constant bound, but chooses parameter #281

Array bounds inference:

Every variable (both pointers and scalars) are identified by a unique number i.e., BoundsKey, There is a mapping from each BoundsKey to the corresponding ProgramVar* which contains information about program scope, variable name, etc.

The main entry point in this class is: bool AVarBoundsInfo::performFlowAnalysis(ProgramInfo *PI): This as the name suggests performs flow (i.e., LockSmith based) analysis to determine the bounds of all the array variables.

It has a fixed point loop, at each iteration, we try to find the possible bounds for all array variables (that do not have bounds) from its predecessors using the following function:

bool AvarBoundsInference::predictBounds(BoundsKey K,
                                        std::set<BoundsKey> &Neighbours,
                                        AVarGraph &BKGraph)

where K is the array variable whose bounds need to be computed and Neighbours are the predecessors and BKGraph is the graph that contains all the flow information. The possible bounds for K will be stored into CurrIterInferBounds.

Later we call convergeInferredBounds that will pick the best bounds from the possible bound values, with the following preference: count will be the priority over byte bounds. Variables will be given priority over constants. If there are multiple choices (i.e., variables or constants) we give up.

We first propagate all the bounds information from explicit declarations and mallocs.
For other variables that do not have any choice of bounds, we use potential bounds choices (FromPB), these are the variables that are upper bounds to an index variable used in an array indexing operation.
For example:

if (i < n) {
   ...arr[i]...
}

In the above case, we use n as a potential count bounds for arr.
Note: we only use potential bounds for a variable when none of its predecessors have bounds.

Function of interest:

AvarBoundsInference::getReachableBoundKeys(const ProgramVarScope *DstScope,
                                                BoundsKey FromVarK,
                                                std::set<BoundsKey> &PotK,
                                                AVarGraph &BKGraph,
                                                bool CheckImmediate)

The function: getReachableBoundKeys finds all the BoundsKeys (i.e., variables) in scope DstScope that are reachable from FromVarK in the graph BKGraph. All the reachable bounds key will be stored in PotK.

[mwhicks1]

Why do you need to call clear() here? Isn't just creating the variable calling its constructor, which will ensure that it's empty?

[Machiry]

Agree. Will remove it.

[mwhicks1]

Seems odd that this should be c since the index of 3 is not at all related to c. I.e., the index is unconditional.

[mwhicks1]

Maybe this is because of the heuristic (which we are not disabling) ?

[Machiry]

This is not a heuristic. It is from all the incoming array pointers i.e., p, q. i.e., all the calls to this function pass array to x and bounds to c. We do not worry about the indexing x as Checked C dynamic checks will take care of that.

[mwhicks1]

Huh?

[mwhicks1]

This is pretty weird, since l is not initialized. Maybe make l the parameter to bar (this function) ?

[Machiry]

Will initialize l to something. I just wanted to check Variation 3

[mwhicks1]

What about also a test where you pass in n as the second parameter, ie. as if it was q's length, even though it's associated with p? The result should be that it's not treated as the length.

[Machiry]

Sure. Will add it.

[mwhicks1]

Returning &x where x is a local variable is bad, isn't it? Same with what's below.

[Machiry]

Its a reference. So, returning &SS where SS is a reference is as good as returning a pointer.

[mwhicks1]

Seems like you completely changed this file. I'll look at it from scratch. Can you say at a high level what it does (I think it does the propagation/inference process) and what its entry point is? Then I will start looking at it from that entry point.

[Machiry]

Will do.

[mwhicks1]

I am wondering: Why do we need different graphs? Why not always use the context-sensitive graph?

[Machiry]

This can be better explained in a call. Reference: https://drive.google.com/file/d/1U4_OLmur0OPlr3iq4CF6GFARv7EK70dg/view?usp=sharing

[mwhicks1]

This is part of keepHighestPriorityBounds -- what makes a possible bounds high priority?

[Machiry]

This is the priority order we use:

checkedc-clang/clang/include/clang/CConv/AVarBoundsInfo.h

Line 83 in dd73a00

enum BoundsPriority {

[mwhicks1]

What is a "potential bound" (fromPB) ?

[Machiry]

As mentioned in the pull request:

For other variables that do not have any choice of bounds, we use potential bounds choices (FromPB), these are the variables that are upper bounds to an index variable used in an array indexing operation.
For example:

if (i < n) {
   ...arr[i]...
}

In the above case, we use n as a potential count bounds for arr.
Note: we only use potential bounds for a variable when none of its predecessors have bounds.

[mwhicks1]

Where does InfABnds get set? I didn't notice it in the performXXX functions.

[Machiry]

It gets set in predictBounds function

[mwhicks1]

When I run the tests on my Mac, I get two unexpected failures.

********************
Failing Tests (2):
    Clang :: CheckedCRewriter/basic.c
    Clang :: CheckedCRewriter/multivardecls.c

  Expected Passes    : 349
  Expected Failures  : 7
  Unexpected Failures: 2

Are these unexpected failures to be expected? :)

[mwhicks1]

I tried this example:

void foo(int *p) {
  p[2] = 1;
}
void bar(int n) {
  int *x = malloc(sizeof(int) * n);
  int y[10];
  foo(x);
  foo(y);
}

It incorrectly rewrites the p count to be 10 even though x, which is passed in, has a length that depends on an arbitrary n. It seems to me that you want to be looking at constraints inside of foo, to notice that count(p) > 2. Then, if you don't have a consistent set of flows in to foo (which you don't, since 10 and n both flow there as possible lengths), you can use the interior constraints to choose a size. Here, it would be count(3). Is this possible to do?

[mwhicks1]

Another question. For this example:

void foo(void) {
  int x[10];
  int *y = x;
  int *z = y;
  z[1] = 1;
}

It produces this graph:

Why are the edges bidirectional? We have bidirectional edges in the ptyp graph to make sense of the kind of pointer it is, but now that we've determined that type, I'm wondering why we don't have single-direction edges for lengths?

[mwhicks1]

As a consequence of the above, I notice that if we add the following line at the end of foo:

  z = malloc<int>(sizeof(int)*5);

then y no longer gets a count added on, and z gets 5. The latter is right, but I would want the former to be 10, still?

Maybe the reason they have to be bidrectional is somehow due to being able to mutate the variables?

[mwhicks1]

A bunch of other random questions:

• What is a BoundsKey doing? It seems like it’s the core of a constraint, like a VarAtom; is that right?
• What is a ProgramVar? Is it literally always a variable, or can it be, for example, a constant? If the latter, perhaps it’s the wrong name?
• What's a CtxFunctionArgScope exactly? I think of scopes as lexical blocks in which variable names are resolved. So a StructScope is the set of fields inside a struct definition; a FunctionParamScope is the set of parameters in a function definition; a FunctionScope is the set of variables defined within a function (including its parameters); etc. There is no particular scope for a context-sensitive call, is there? Maybe you are just looking at the variables that are used at a particular call site? E.g., if I make a call foo(a,b) and one place and foo(b,c) at another, there would be two scopes, with a and b in the first and b and c in the second?
• What is a GlobalScope used for?

[Machiry]

As a consequence of the above, I notice that if we add the following line at the end of foo:

  z = malloc<int>(sizeof(int)*5);

then y no longer gets a count added on, and z gets 5. The latter is right, but I would want the former to be 10, still?

Maybe the reason they have to be bidrectional is somehow due to being able to mutate the variables?

Yes, this is a consequence of having bi-directional edges and we want the inferred bounds to be consistent.
So, bounds of y will be conflicting i.e., 10 vs 5 and we give up.
However, we consider malloc as seed bounds (i.e., ground truth) i.e., similar to declared bounds, and hence z gets the bounds.

[Machiry]

Another question. For this example:

void foo(void) {
  int x[10];
  int *y = x;
  int *z = y;
  z[1] = 1;
}

It produces this graph:

Why are the edges bidirectional? We have bidirectional edges in the ptyp graph to make sense of the kind of pointer it is, but now that we've determined that type, I'm wondering why we don't have single-direction edges for lengths?

I wanted the inferred bounds to be consistent i.e., for a given array pointer p, all other array pointers (that have bounds) and are related (flow into p or flow from p) should agree with bounds.

We can do better if we have flow-sensitive bounds...which will be supported by Checked C eventually!

[Machiry]

I tried this example:

void foo(int *p) {
  p[2] = 1;
}
void bar(int n) {
  int *x = malloc(sizeof(int) * n);
  int y[10];
  foo(x);
  foo(y);
}

It incorrectly rewrites the p count to be 10 even though x, which is passed in, has a length that depends on an arbitrary n. It seems to me that you want to be looking at constraints inside of foo, to notice that count(p) > 2. Then, if you don't have a consistent set of flows in to foo (which you don't, since 10 and n both flow there as possible lengths), you can use the interior constraints to choose a size. Here, it would be count(3). Is this possible to do?

This is because at call site foo(x), the context-sensitive argument does not have bounds.
We see x(n) -> x_ctx_sens_p(unknown bounds)->p and y(10) -> y_ctx_sens_p(10)->p.

Our inference looks for pointers that have bounds, here, in this above case, we only see y_ctx_sens_p as the array that has bounds and try to infer bounds from it.

[Machiry]

When I run the tests on my Mac, I get two unexpected failures.

********************
Failing Tests (2):
    Clang :: CheckedCRewriter/basic.c
    Clang :: CheckedCRewriter/multivardecls.c

  Expected Passes    : 349
  Expected Failures  : 7
  Unexpected Failures: 2

Are these unexpected failures to be expected? :)

Not expected. Will take a look.

[Machiry]

When I run the tests on my Mac, I get two unexpected failures.

********************
Failing Tests (2):
    Clang :: CheckedCRewriter/basic.c
    Clang :: CheckedCRewriter/multivardecls.c

  Expected Passes    : 349
  Expected Failures  : 7
  Unexpected Failures: 2

Are these unexpected failures to be expected? :)

Not expected. Will take a look.

Looks like it is missing header files. Can you see the exact error you are seeing? you can use llvm-lit -v to get all the warnings.

[mwhicks1]

@jackastner found and fixed the multivardecls.c bug. It's been merged as of yesterday.

For the basic.c one, I see this as the expected output in basic1:

char data _Nt_checked[17] = "abcdefghijklmnop";

but on my machine I get

char data _Checked[17] = "abcdefghijklmnop";

[mwhicks1]

OK, merge when ready.

My last request is that your comment at the top is very helpful in describing how things work. It would be useful to add it as comments in the code. For example, you could add a comment at near PerformFlowAnalysis about it being the entry point, and using the opportunity to describe how it works, there.