Re: gl_linked_iterator doesn't initialize all fields.

[Bruno Haible]

Hi Collin,

> Here is the function that it is warning about:
> 
> static gl_list_iterator_t _GL_ATTRIBUTE_PURE
> gl_linked_iterator (gl_list_t list)
> {
>   gl_list_iterator_t result;
> 
>   result.vtable = list->base.vtable;
>   result.list = list;
>   result.p = list->root.next;
>   result.q = &list->root;
> #if defined GCC_LINT || defined lint
>   result.i = 0;
>   result.j = 0;
>   result.count = 0;
> #endif
> 
>   return result;
> }

This code can be described as:
  "We initialize the result fields that are necessary to initialize.
   We know that some tools report that i, j, count are uninitialized,
   therefore we give you a way to silence the tools' reports: namely,
   compile with -DGCC_LINT."

> When compiling with -fanalyzer and including gl_anylinked_list2.h I
> see this:
> 
> In file included from gl_linked_list.c:29:
> gl_anylinked_list2.h: In function 'gl_linked_iterator':
> gl_anylinked_list2.h:952:10: warning: use of uninitialized value 
> 'result.count' [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
>   952 |   return result;
>       |          ^~~~~~
>   'gl_linked_iterator': events 1-3
>     |
>     |  940 |   gl_list_iterator_t result;
>     |      |                      ^~~~~~
>     |      |                      |
>     |      |                      (1) region created on stack here
>     |      |                      (2) capacity: 8 bytes
>     |......
>     |  952 |   return result;
>     |      |          ~~~~~~       
>     |      |          |
>     |      |          (3) use of uninitialized value 'result.count' here

This is a false alarm.

Different tools exist for determining uninitialized values:
  - valgrind is the tool of choice. It has some false alarms, which
    can be silenced through *.valgrind files. But other than that,
    it is rock-solid.
  - gcc and clang have some data flow analysis. They can report uninitialized
    values. gcc with -fanalyzer has many false alarms. In particular, from
    this warning, you can see that it considers a 'return' statement of a
    struct to be a "use" of all fields. Which is wrong.

> I couldn't find the reasoning why these were hidden behind 'lint' [1].

In Gnulib, we follow the approach that Paul has developed over the years:

  1) First, determine whether the warning pinpoints a problem or is a false
     alarm.

  2) In case of a problem, fix the problem in the code.

  3) In case of a false alarm, report it as a GCC bug and optionally add a
     GCC_LINT conditional, so that
       - users can silence the warning if they want to and if the warning
         category is generally somewhat useful,
       - readers of the code can see that the problem was already handled.

The rationale of this approach is:

  * The code is ultimately what we want to maintain for 20 and more years.
    Whereas the capabilities of static analyzers evolve over time. It is
    well possible that in 10 years the GCC analyzer has this fixed. If we
    were to throw away the '#if', as you suggest, we would no longer know
    that this piece of code can be optimized.

  * We want to pressure GCC, to improve their diagnostics. If we were to
    just silence the warning by adding unconditional initializations, there
    would not be pressure on GCC.

  * Last not least, we don't want unnecessary code to be executed. We are
    programming C, not C++, precisely because we want to keep control over
    which code gets executed.

> But my instinct tells me these initializations should be done
> unconditionally so undefined behavior is avoided.

There is no undefined behaviour here, only a false alarm.

Bruno