memmem.c -Wpointer-bool-conversion warning

[ghost]

Unfortunately -Werror is enabled by default, and when compiled with gcc-6.1 or clang-3.8, the following warning will prevent a successful build:

memmem.c:73:14: warning: nonnull parameter 'b1' will evaluate to 'true'
  on first encounter [-Wpointer-bool-conversion]
        if(!(b1 && b2 && len1 && len2))
             ^~ ~~
memmem.c:73:20: warning: nonnull parameter 'b2' will evaluate to 'true'
  on first encounter [-Wpointer-bool-conversion]
        if(!(b1 && b2 && len1 && len2))
                ~~ ^~

[achlipala]

It's intentional that -Werror is on by default, and I'm always glad to have it catch cases that start bothering popular compilers. I believe I've fixed this particular issue. Can you please confirm?

[ghost]

4a95c63 turned it into:

memmem.c:73:14: warning: comparison of nonnull parameter 'b1' not
equal to a null pointer is 'true' on first encounter
[-Wtautological-pointer-compare]
        if(!(b1 != NULL && b2 != NULL && len1 != 0 && len2 != 0))
             ^~    ~~~~
memmem.c:73:28: warning: comparison of nonnull parameter 'b2' not
equal to a null pointer is 'true' on first encounter
[-Wtautological-pointer-compare]
        if(!(b1 != NULL && b2 != NULL && len1 != 0 && len2 != 0))

[achlipala]

Hm... do you happen to know the appropriate fix?

[achlipala]

My next guess would be to move that line to the beginning of the function. Does that work for you?

[ghost]

Makes no difference. See llvm-mirror/clang@01ad089.

As a last resort, you could enable -Wno-tautological-pointer-compare for memmem.c.

[achlipala]

Hm... I don't understand why those pointers should be tautologically non-null at the beginning of the function, before the variable declarations. (Later on, it makes sense to assume they're non-null, since otherwise undefined behavior would be triggered by some dereferences.) I would like to understand what the compilers are complaining about and fix it properly!

[ghost]

To avoid an error on my part, this is what you wanted me to try, right?

diff --git a/src/c/memmem.c b/src/c/memmem.c
index 891b211..99163dd 100644
--- a/src/c/memmem.c
+++ b/src/c/memmem.c
@@ -60,6 +60,10 @@ __RCSID("$NetBSD$");
 void *
 memmem(const void *b1, size_t len1, const void *b2, size_t len2)
 {
+        /* Sanity check */
+        if(!(b1 != NULL && b2 != NULL && len1 != 0 && len2 != 0))
+                return NULL;
+
         /* Initialize search pointer */
         char *sp = (char *) b1;

@@ -69,10 +73,6 @@ memmem(const void *b1, size_t len1, const void *b2, size_t len2)
         /* Intialize end of search address space pointer */
         char *eos   = sp + len1 - len2;

-        /* Sanity check */
-        if(!(b1 != NULL && b2 != NULL && len1 != 0 && len2 != 0))
-                return NULL;
-
         while (sp <= eos) {
                 if (*sp == *pp)
                         if (memcmp(sp, pp, len2) == 0)

[achlipala]

Right.

[bbarenblat]

This occurs because in some libcs (including glibc), string.h declares memmem with nonnull attributes on the pointer arguments, e.g.,

extern void *memmem (const void *__haystack, size_t __haystacklen,
                     const void *__needle, size_t __needlelen)
     __THROW __attribute_pure__ __nonnull ((1, 3));

If a declaration like this is in scope, Clang assumes that the pointer arguments are nonnull, and it helpfully warns that the user may have made a mistake somewhere.

I’d prefer to fix this by renaming our memmem function to something else, like urweb_memmem. This would ensure that we don’t accidentally capture functions from the libc. Disabling the warning for that function is also reasonable, though it may have unintended consequences – notably, the compiler will still assume that the pointers are nonnull and will probably optimize away the NULL checks.

[achlipala]

Thanks for the diagnosis. I've pushed a name change.

@Tuncer, can you confirm that the name change fixes the issue for you?

[ghost]

Thanks, confirmed.

[ghost]

Just a thought: should the file be renamed (memmem.c -> ur_memmem.c)?

[achlipala]

Hm... it doesn't seem too important to optimize the file name, since it won't even be preserved in the compiled library, outside of debug symbols.