Checking Code Quality Warnings: Highlighting More Potential Bugs
June 25, 2019
by Randy Brukardt

Previously, I had described the intent and details of the new Quality Warnings found in Janus/Ada 3.2.1 (see Code Quality Warnings: Highlighting Likely Bugs before They Bite). The original set of warnings did not include the access value patterns suggested in the motivating AdaCore blog item. These were omitted as they could not be easily implemented with the information available in the Janus/Ada front-end.

Specifically, one of the patterns looks like:

   if P = null then
      ... P.all ...
   end if;

In this code, the null access value check for the dereference of P will always fail and raise Constraint_Error. Unless this is an ACATS or similar test, this is a bug.

The Janus/Ada compiler generally processes a single expression at a time. This is in part a result of its heritage as a small host compiler – processing one expression at a time uses less memory than keeping larger structures in memory.

However, the access value pattern given above contains two separate expressions, and they can be separated by a long distance (an arbitrary number of other expressions and nesting could be between them). This makes it impractical to detect a pattern like this in the Janus/Ada front-end outside of the most trivial cases.

The Janus/Ada optimizer usually has access to larger portions of code. Moreover, the optimizer can check most causes of this sort of problem, rather than just a specific pattern, giving broader coverage for the warning. It also already supports many of the operations needed to determine if this problem is truely present, reducing the number of false positives. Therefore, we looked at supporting this warning within the optimizer. One problem with doing so is that the optimizer was not set up to report warnings or errors. Location information in the intermediate code generally only identifies the starting line of an expression; more precision is clearly needed for an understandable quality warning (especially as it often will be reporting on a check associated with an implicit dereference or a subtype conversion not visible in the source code). This problem caused us to leave these warnings out of our initial set of Quality Warnings.

Quality Code will typically be written such that language-defined checks cannot fail. For instance, for access types, either null excluding subtypes or explicit checks will be used to ensure that an access value cannot be null. Such code will be a lot more common than quality bugs. Building the quality check into the optimizer also adds the possibility of eliminating the code for checks that can be proven to be unneeded. This can be even more valuable than the quality warnings, as it improves the resulting code and makes it less necessary to suppress checking.

Further thought on this topic demonstrates that these points apply to almost all language-defined checks. One would often want to protect discriminant checks, index checks, even assertion checks with either explicit code or subtypes. And if that is done, analysis of the code can possibly remove the checks by proving they cannot fail, prove a likely bug, or neither.

Therefore, we determined to create a second set of Quality Warnings, the Checking Quality Warnings. These are implemented in the optimizer, in part by associating detailed location information with each language-defined check. We started the implementation with access checks, as these have the fewest possible states to account for. This let us use access checks to prove the concepts. For each associated check, the optimizer determines if the check cannot fail, must fail, cannot be executed, or that it cannot determine any of these. All of these results except the first are associated with a warning, using the provided warning location to precisely identify the check.

The following function illustrates all of these cases:

   type My_Access is access Natural;

   function Get (P : My_Access) return Natural is
      T : Natural;
   begin
      T := P.all; -- (1)
      if P /= null then
          T := P.all; -- (2)
      elsif P = null then 
          T := P.all; -- (3)
      else
          T := P.all; -- (4)
      end if;
      return T;
   end Get;

(1) is the case where the optimizer cannot tell whether or not the access check will be triggered. This will generate a warning that a "null check may fail". (2) is the correct quality code, and no warning will be generated nor will any check code be generated. (3) is the case where the check will always fail. A warning that the check will always fail will be generated. This is a bug of some sort – the condition could be wrong, the code could be in the wrong if branch, or there may be some other problem.

Finally, (4) is the case where the check cannot be executed. A warning about unreachable code is generated (as opposed to the usual informational message). We consider this case more severe than the usual unreachable code, as it cannot be caused by intentional conditional compilation.

These warnings are implemented in Janus/Ada 3.2.1. These work well, other than that for the "null check may fail" warning is sometimes reported when it is obvious that it should not be (a "false positive").

The most common case of a false positive is when a subprogram parameter is assumed to not be null, but this is not declared in the code using a null exclusion. This is easily fixed by adding a null exclusion (not null) directly to the declaration of the formal parameter (a separate subtype is not needed). The check will then be done at the site of the call, which should be easier to verify. Of course, the same protections need to be applied to the actual parameter(s) in that case.

A similar case occurs for temporary objects which are assumed to be not null. This is also easily fixed by adding a null exclusion (not null) directly to the declaration of the temporary object. The check will then be done when the temporary object is assigned.

Implementing these warning using the optimizer means of course that the warnings are limited by the limitations of the optimizer. This causes additional forms of false positive. In most of these cases, some restructuring will eliminate the issues.

For instance, the optimizer will not analyze code across exception handler boundaries, in order to properly implement 11.6 rules. That means that in an example like:

   if P /= null then
       begin
           T := P.all; -- (5)
       exception
           when Constraint_Error => null;
       end;
   end if;

Even though the test and use are close together, they are separated by an exception handler. Ada limits the motion of code across handlers, and Janus/Ada handles this by optimizing the code before, during, and after the exception handler applies separately. This causes a false positive at (5). The solution for a case like this is to introduce a null excluding object to move the check outside of the handler:

   if P /= null then
       declare
           Temp_P : not null My_Access := P;  
       begin
           T := Temp_P.all;
       exception
           when Constraint_Error => null;
       end;
   end if;

The null check is now outside of the exception handler and now should visible to the optimizer along with the condition. The dereference does not need a null check since the subtype excludes null, so no "may fail" warning will occur.

If there is an individual check whose warning just cannot be eliminated, but can be determined to be safe by other means, pragma Warning_Level can be used to eliminate the warning in a small area. Warning_Level works much like Suppress, setting the warning level until the end of the enclosing scope. Since the "may fail" message has warning level 6, setting the warning level to 5 will hide it in a particular scope. So, for instance, we could have changed our handler example as follows to get rid of this particular warning but still check for it in other code:

   if P /= null then
       declare
           pragma Warning_Level (5);
       begin
           T := P.all;
       exception
           when Constraint_Error => null;
       end;
   end if;

This is especially useful if the Warnings are Errors mode is used, as any detected warning will prevent the unit from compiling.

The Janus/Ada optimizer is not very smart about what gets modified by calls and indirect writes. This can cause additional false positives. For instance, a call generally is considered to kill all values (that is, potentially change them), so the checking optmizations usually cannot cross a call. That means something like:

   if P /= null then
       Ada.Text_IO.Put("Got here");
       T := P.all; -- (6)

The call will cause a false positive at (6). Rearranging the code or introducing a null-excluding temporary can eliminate these.

A similar effect can happen for an indirect write:

   if P /= null then
       P.all := 10; -- (7)
       T := P.all; -- (8)

A false positive will occur at (8), even though it is obvious (to us) that P is not modified by the assignment to P.all.

We were already planning to make the optimizer smarter in some of these cases, and this effect with the checking warnings will increase the priority of doing that.

With the current version of Janus/Ada, it is often the most useful to use /w5 with /yqc. That turns on the checking quality warnings, but suppresses any warnings about checks that might (but might not) fail. We're using this configuration to compile Janus/Ada; with code not originally designed for these checks, there are thousands of checks where access values are assumed to not be null that aren't documented or asserted. Wading through all of those is unlikely to improve the code much (it's been tested throughly for decades). But getting the warnings for always failing checks and unreachable checks will improve the compiler, as well removing unneeded checks in the case where the compiler can tell that a check cannot fail.

We're still considering how the "check may fail" case should be reported, and whether the check quality warnings should be turned on with the optimizer (rather than separately as is currently the case). Given the value of the always fail and unreachable warnings, perhaps they should be on most of the time. Perhaps the only warning that really needs to be controlled is the "check may fail" warning. More thought and practice is needed on this point.

In future versions of Janus/Ada, we intend to extend this mechanism to most integer-related checks, as well as implementing a similar mechanism for assertions (including Pre and Post).

We'd like to hear from any Janus/Ada user on how the Quality Warnings are working for them. Write me at randy@rrsoftware.com.