Re: [PATCH] warnings: address -Wnull-dereference in reader.c

[Akim Demaille]

Hi Paul,

Great to see you here!

> Le 15 août 2018 à 18:34, Paul Eggert <address@hidden> a écrit :
> 
> Akim Demaille wrote:
>> will C stop requiring
>> void some day?
> 
> It’s not likely any time soon, since K&R-style function definitions are still 
> allowed in C11 and even if they're removed it'll likely require two 
> iterations of the standard to do it.

Bummer…

>> +  argmatch assert assume
> 
> 'assume'? There is no assume module in gnulib; the 'assume' macro is defined 
> by verify.h, in the 'verify' module. If reader.c uses ‘assume' it should 
> include verify.h.

Good catch!

>> static symbol *
>> -find_start_symbol ()
>> +find_start_symbol (void)
>> {
>>   symbol_list *res = grammar;
>>   for (;
>> -       res != NULL && symbol_is_dummy (res->content.sym);
>> +       res && symbol_is_dummy (res->content.sym);
>>        res = res->next)
>>     {
>>       for (res = res->next;
>> -           res != NULL && res->content.sym != NULL;
>> +           res && res->content.sym;
>>            res = res->next)
>>         continue;
>> -      aver (res != NULL);
>> +      assume (res);
>>     }
>> -  aver (res != NULL);
>> +  assume (res);
>>   return res->content.sym;
>> }
> 
> This doesn't look right, as 'assume (X)' is an instruction from the 
> programmer to the compiler, saying "I know that X is nonzero even though you 
> don't, so compile the code assuming that X is nonzero even if the compiled 
> code would be incorrect if X were zero."  And yet here, you have a for loop 
> that explicitly exits the loop if res is null, and then says 'assume (res)'. 
> Either the test for res is wrong, or the assume is wrong.
> 
> If you already know that the start symbol is in the grammar, then write the 
> code that way; there is no need to test whether res is nonzero. That way, you 
> will probably be able to pacify GCC without using ‘assume' at all.

You are 100% right, we should be ‘less cautious’ as it
allows the compiler to understand better what we assume.

Thanks for the tip!  What do you think about this?

commit fab0bf6b521bd524751605d263dd6bd7e2426099
Author: Akim Demaille <address@hidden>
Date:   Wed Aug 15 21:03:47 2018 +0200

    reader: simplify the search of the start symbol
    
    Suggested by Paul Eggert.
    
    * src/reader.c (find_start_symbol): Don't check 'res', we know it is
    not null.  That suffices to avoid the GCC warnings.
    * bootstrap.conf: We don't need 'assume', which doesn't exist anyway.

diff --git a/bootstrap.conf b/bootstrap.conf
index b2d0f974..7c436a59 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -17,7 +17,7 @@
 
 # gnulib modules used by this package.
 gnulib_modules='
-  argmatch assert assume
+  argmatch assert
   calloc-posix close closeout config-h c-strcase
   configmake
   dirname
diff --git a/src/reader.c b/src/reader.c
index 12faef96..664dea71 100644
--- a/src/reader.c
+++ b/src/reader.c
@@ -729,17 +729,11 @@ static symbol *
 find_start_symbol (void)
 {
   symbol_list *res = grammar;
-  for (;
-       res && symbol_is_dummy (res->content.sym);
-       res = res->next)
-    {
-      for (res = res->next;
-           res && res->content.sym;
-           res = res->next)
-        continue;
-      assume (res);
-    }
-  assume (res);
+  /* Skip all the possible dummy rules of the first rule.  */
+  for (; symbol_is_dummy (res->content.sym); res = res->next)
+    /* Skip the LHS, and then all the RHS of the dummy rule.  */
+    for (res = res->next; res->content.sym; res = res->next)
+      continue;
   return res->content.sym;
 }