Handling function pointer bounds

[Machiry]

• Make it explicit to not handle bounds for parameters of function pointers.
• Fix writing of bounds to parameters of function pointers.

Issue: #575

[mattmccutchen-cci]

@Machiry This change may stop John's particular example from crashing 3C, but I wouldn't be surprised if there are other places that 3C can dereference a null ProgramVar given that it has already happened twice (#523 and #575). Can you explain more generally under what conditions a ProgramVar is allowed to be null and how the code is intended to prevent null dereferences so we can try to fix more of the problems now rather than later?

[Machiry]

@Machiry This change may stop John's particular example from crashing 3C, but I wouldn't be surprised if there are other places that 3C can dereference a null ProgramVar given that it has already happened twice (#523 and #575). Can you explain more generally under what conditions a ProgramVar is allowed to be null and how the code is intended to prevent null dereferences so we can try to fix more of the problems now rather than later?

ProgramVar is an object for each program variable (pointers and non-pointers) maintained by array bounds inference code. Array bounds code works with BoundsKey (unique id for all entities involved in bounds inference).

Ideally, all Boundskeys should have corresponding ProgramVar. However, there are certain cases where this is not the case. It is hard to anticipate these cases because of the crazy C syntax.
IMO, a reasonable approach is to fix these null-ptr issues as they come.

[john-h-kastner]

The total number of calls to getProgramVar is fairly low (22 according to clion). Could it make sense to just add a null check on all of them? The concern I suppose would be that this silently swallows an error in bounds inference that caused the program var to be absent in the first place.

[mattmccutchen-cci]

The total number of calls to getProgramVar is fairly low (22 according to clion). Could it make sense to just add a null check on all of them? The concern I suppose would be that this silently swallows an error in bounds inference that caused the program var to be absent in the first place.

I suggest we make this a warning; then if we notice incorrect behavior later, we'll see the warning as a possible cause. If we later make it a priority to fix problems of this nature, we could run whatever tests we want with the warning temporarily changed to an error (via a command-line flag or just editing the code on a branch). This case with getProgramVar may be a good opportunity to set up a convenient mechanism that we can use in similar situations in the future.

[Machiry]

The total number of calls to getProgramVar is fairly low (22 according to clion). Could it make sense to just add a null check on all of them? The concern I suppose would be that this silently swallows an error in bounds inference that caused the program var to be absent in the first place.

I suggest we make this a warning; then if we notice incorrect behavior later, we'll see the warning as a possible cause. If we later make it a priority to fix problems of this nature, we could run whatever tests we want with the warning temporarily changed to an error (via a command-line flag or just editing the code on a branch). This case with getProgramVar may be a good opportunity to set up a convenient mechanism that we can use in similar situations in the future.

I would rather not make this, getProgramVar returning NULL is not expected (except for few cases). Making this a warning could silently skip these and I do not feel comfortable with that. Let's see if we face another of these NULL pointer issues and then make it as a warning.

[mattmccutchen-cci]

I discussed the general unreliability of the bounds inference code with Mike. It's not clear to us what we should do about it now, so we propose to proceed with this specific fix and we can revisit the general problem later. John, if you have other thoughts about the general problem, let us know. Otherwise, we need to decide whether John or I will review the PR on its own merits. I imagine John knows more, but at some point it may make sense for me to start learning.

[john-h-kastner]

Looks fine. Mostly suggestion for comment improvements.

[john-h-kastner]

Shouldn't this always be true now? isValidBoundVariable returns false when getParentFunctionOrMethod is null, so execution would never reach here.

[Machiry]

Actually, No. isValidBoundVariable does not check for getParentFunctionOrMethod.

bool AVarBoundsInfo::isValidBoundVariable(clang::Decl *D) {
  // All parameters, return, and field values are valid bound variables.
  if (D && (isa<ParmVarDecl>(D) || isa<FunctionDecl>(D) || isa<FieldDecl>(D)))
    return true;
 
  // For VarDecls, check if these are are not dummy and have a name.
  if (auto *VD = dyn_cast_or_null<VarDecl>(D))
    return !VD->getNameAsString().empty();

  return false;
}

[john-h-kastner]

This should be fine to merge.