Sun Lint notes

Integrating Sun lint into the library build as additional files to be built in a normal appears to be fairly easy, since -D and -I options for the compiler apply equally well to lint, and we can just build .ln files parallel to the .so files we build for libraries. I haven't yet tried making "make lint" a separate target, nor running lint for the application programs; the last is where some of the more advanced warnings would seem to come in. But it looks like it's going to require tweaking every makefile that builds a program on UNIX.

There seems to be considerable, though not complete, overlap between things reported by lint and things reported by gcc with appropriate warnings enabled. (Many of which we're already ignoring with gcc.)

There are extra warnings gcc doesn't seem to be producing which may be of interest. For example:

• header file included but not needed
• variable set but not used
• assignments causing narrowing of integral type
  • gcc warns in function-call cases with -Wconversion, but not assignments

Some of the warnings from lint will impose additional coding style
restrictions on us, for example:

• Use "for (;" instead of "while (1)".
  • Lint complains about a constant condition in the latter, and in one case, seemed to think that the following statement could be reached after "while (1)" but not with "for (;".
• Use "if ((a = foo()) != 0)" instead of "if ((a = foo()))".
  • Note that the extra parens in the latter are a style imposed by gcc to suppress the similar warning it generates.
• Certain conditional constructs that may boil down to "do nothing" or "use this constant" on only some platforms may need special crafting to avoid warnings in lint, and avoiding warnings in both lint and gcc may take some work, or even conditionalizing on "#ifdef lint".
• The "do
  Unknown macro: { ... }
  while (0)" construct doesn't seem to be accepted as a way to turn multiple statements into one. Lint complaints about the constant condition, and also complains about not reaching the end-of-loop code if the wrapped code includes a return statement.

There are some pretty obvious false positives, like "modification using a NULL pointer" reported for "if (usecptr) *usecptr = now.tv_usec" in lib/krb4/unix_time.c.

Lots of "BAD TYPE #####" strings dumped out in the "inconsistent use of a value type" report.

It appears that comments to suppress warnings don't work inside macro definitions. E.g.,

#define ret() do

/CONSTCOND/ while (0)

causes warnings, though if the expansion is used directly, both warnings are suppressed. So we need to use the NOTE() form, which means conditionally pulling in note.h, defining away NOTE (or a macro of our own) when not running Solaris lint, etc. Or, suppress these warnings globally.

Lint warnings can be suppressed by type on the command line, in certain cases by type for a function or expression via comments, and (I think) in all cases for a specific line via comments. I think we could, if we wanted, suppress some or all of the things that GCC also warns about, so we only get one set of coding style restrictions imposed on us. I'd only suggest doing this on a case by case basis, and only if coding to satisfy both tools gets too annoying.

Lint doesn't seem to understand "library exports only some symbols", but I haven't really experimented with that yet.

Do we have a channel by which to report bugs or request enhancements in lint?

Suppressions I've enabled throughout in my test build:

• E_STATIC_UNUSED, E_UNCAL_F - complains about static inline functions
• E_CONSTANT_CONDITION - complains because K5_PTHREADS_LOADED is a constant on Solaris, but won't be elsewhere, and we test it. We can't attach a "CONSTCOND" note to some definitions of K5_PTHREADS_LOADED, or at least not with the ways I've tried. It appears that it needs to be used elsewhere in the test.
• E_H_C_CHECK0,E_H_C_CHECK1,E_H_C_CHECK2 - We don't use the coding style that each .c file has a corresponding .h file with the matching name, and the global definitions in foo.c must match the declarations in foo.h.
• E_MCR_NODIFF - We define some macros differently in different files, especially md4 vs md5.
• E_INCL_NUSD, E_INCL_MNUSD - Many cases are system headers needed on some systems (e.g., where is int32_t defined?), or krb5 headers with extern function decls we want to check consistency with.
• E_PTRDIFF_OVERFLOW - Complains about every char pointer subtraction?
• E_P_FREE_NULL_PSBL - Passing a null pointer to free is okay.
• E_P_USE_DEALLOC_PSBL, E_P_USE_DEALLOC? - Lint assumes realloc always frees its pointer argument, but we often attempt to handle the allocation failure case, where it's specified not to do so.
• E_ASGN_RESET - Analysis bugs seem to be causing a number of false positives.
• E_P_USE_SUSP_PSBL - So far, the error location reports seem almost impenetrable. The displayed lines of code often refer to unrelated fields, sometimes having integer computations that don't seem related to the "possible use of a pointer produced in a questionable way". They may bear investigating at some point, but I wouldn't recommend this option for general use.

Function-style macros that "return" values that we ignore in some places (e.g., our macro for setting F_CLOEXEC on a file is written to return zero or errno, but we usually – always? – ignore the result) can generate "sub-expression has null effect" warnings, whereas ignoring function return values does not. Casting to void at the call site suppresses this, as presumably would tweaking the macro to do the cast. If we sometimes care, this could be annoying. Maybe in some cases the macro can be restructured so that the final value is the return value of a function call; lint seems to be okay with ignoring those.

It appears that the "LINTED" comment to suppress general warnings cannot be a block comment; it must be a one-line comment.
/* LINTED This comment does not suppress a warning in the code on
the following line; it generates an additional warning. */
/*

• LINTED This is totally ignored.
  */
  Between indentation-level and line-length specs in our coding standards, this could significantly constrain what we could express in these messages.

Avoid empty branches in if statements ("{}" or "

") to avoid warning ("statement has no consequent: if"); non-debug versions of macros used as statements can use (void)0 as explicit do-nothing.

Avoid assigning to variable when the value of the assignment expression is the only time the variable's value is used. ("set but not used")

Be careful of "include file is unnecessary" warnings. They come up when the only reason for including the header is to get the public declaration of a function defined in the current source file. Lint considers it unnecessary, gcc (with some warning options) warns if you don't have a declaration for a function. I prefer the latter approach, personally, so I'll probably recommend we suppress it, though it's probably a good idea to review the generated warnings first. Redundant includes (e.g., including stddef.h twice in a row) don't necessarily seem to trigger "is unnecessary" warnings.

Subtracting char pointers appears to always generate an overflow warning, even in cases where simple analysis can show that no overflow is possible. Probably need to suppress this too.

extern char s[40];
ptrdiff_t foo()

We should work out how to best express loops written just to count things or find a null element, where all the useful work is expressed in the "for" header and the body is empty. Either pick something that makes lint shut up, or categorically suppress the empty-loop-body warnings.

(Probably not lint specific.) We have many cases where multiple variables are related – e.g., if foo is 1 then bar has been set and may need to be freed, if foo is 0 then bar is uninitialized. This seems to give both lint and Coverity Prevent (so far) trouble. In fact, this seems to make a lot of the warnings from lint pass 2 false positives.

There are several ways in which error locations in the source can be reported, some brief, and some more detailed showing actual source lines and even macro expansions.

Running pass 1 over library code is easy. Running the full analysis over the KDC sources plus the .ln files generated for the libraries took over 20 minutes (and generated over 40MB of output) before I interrupted it. This is not something we can trivially incorporate into the normal build process.

One of the "possible reference to deallocated memory" warnings I dug into doesn't actually seem to make much sense, BUT the code in which it occurred did have a possible dereference of an uninitialized pointer.

Lots of "possible use of a pointer produced in a questionable way" reports, as noted above often with reference to bits of code with unrelated pointer uses.

General recommendation:

Use pass 1 for library code. Don't try to make the tree pass without warnings initially; instead, pick some of the warnings that correlate to various coding style practices (e.g., warnings about K&R style function declarations; explicitly annotating switch statement branches intended to fall through into the next branch), make them get flagged as errors, and fix the build to work again. The exact list will depend on the coding standards we choose. Review all warnings concerning improper alignment, inconsistent variable or function declarations.

Don't use pass 2 (full program analysis), or at least, use it for tackling only selected error types.

Update: I did not try creating separate lint-library inputs to make the llib-lfoo.ln files; I only tried this with the actual library sources. The model described in some OpenSolaris docs (not the main lint manual page) seems to suggest that this way is preferred – run pass2 over your library files, which I wasn't doing, and separately process llib-lfoo which just includes your header files, to generate llib-lfoo.ln for use with other code.

Doing it this way may add several minutes to the build for libraries, but isn't nearly as bad (or, I suspect, as thorough) as doing full-program processing for multiple executables with all the library data used directly. However, even just working on libraries, it still generates lots of false positives around cases of related variables (e.g., count is nonzero iff ptr has been allocated).

We should split krb5.h into at least two header files. One should encompass all the crypto library exports, and one the krb5 library exports. Then we can easily build llib-lfoo files for each library independently. We may also be able to skip the concatenation steps in the build process.