Re: [PATCH 3/3] lib/regexec: Resolve unused variable

[Darren Kenny]

Hi Paul,

On Wednesday, 2021-08-11 at 00:24:25 -07, Paul Eggert wrote:
> On 6/18/21 8:44 AM, Darren Kenny wrote:
>> The reason for this issue is that we are not building with DEBUG set and
>> this in turn means that the assert() that reads the value of the
>> variable match_last is being processed out.
>
> Unfortunately I don't understand the scenario here. If not building with 
> DEBUG, 'DEBUG_ASSERT (match_last != 1)' should expand to 'assume 
> (match_last != 1)', which in turn should expand to something that 
> evaluates the expression 'match_last != 1'. Please see this commit, 
> which removed the "#ifdef" that you're proposing to re-add:
>
> https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=79f8ee4e389f8cb1339f8abed9a7d29816e2a2d4

The description of the problem by Coverity is:

- An assigned value that is never used may represent unnecessary
  computation, an incorrect algorithm, or possibly the need for cleanup or
  refactoring.  In re_search_internal: A value assigned to a variable is
  never used. (CWE-563)

That it is referring to in this case is that this assignment is being
made to set match_last = -1, but before ever testing it with-in the
for-loop, it is overwiting that value.

So, the only testing was occurring outside of the for-loop, and in what
appeared to be an assertion only fired during DEBUG.

What I did here was to not overwrite the value if DEBUG is set,
since it appeared to never be checked anyway within the for-loop.

I don't know about DEBUG_ASSERT() becoming an assume() - I can't even
find a reference to assume() in the GCC manuals, only assert(),
similarly for the gnulib manual's index at:

- 
https://www.gnu.org/software/gnulib/manual/html_node/Index.html#Index_cp_letter-A

I can see that LLVM has a __builtin_assume() though, but that's about
the closest I could find.

Searching through the gnulib sources did find verify.h and the assume
macro defined in that - I can only assume (pun not intended) that this
is ending up as a no-op for our build, which means that Coverity sees it
as never being read after the for-loop.

Thanks,

Darren