This is an archive of the discontinued LLVM Phabricator instance.

Remove no-op null checks, NFC
ClosedPublic

Authored by vsk on Dec 4 2017, 4:08 PM.

Details

Summary

Null-checking functions which aren't marked 'weak_import' is a no-op
(the compiler rewrites the check to 'true'), regardless of whether a
library providing its definition is weak-linked.

Remove the no-op checks to clean up the code and silence
-Wpointer-to-bool.

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Dec 4 2017, 4:08 PM
jasonmolenda edited edge metadata.Dec 4 2017, 4:23 PM

I don't think that is correct. here's an example in the form of a makefile. the main binary calls foo() which is weak linked from a library. In the first run, foo() is present in the library. In the second run, the library is rebuilt so foo() is absent. The behavior of the program differs.

In this case, we needed an lldb which was built on a system with libcompression to be able to run on a system without libcompression. Although for Apple's purposes, we are no longer supporting lldb on systems without libcompression so it's not really needed any longer for our uses.

all:
echo 'int foo() { return 5; }' > mylib.c
echo "int foo() attribute((weak_import)); int printf(const char *, ...); int main() { if (foo) printf (\"have foo() %d\\\n\", foo()); else printf(\"do not have foo\\\n\"); return 0; }" > main.c
clang -dynamiclib -o libmylib.dylib mylib.c
clang main.c -L. -lmylib
./a.out
echo 'int bar() { return 5; }' > mylib.c
clang -dynamiclib -o libmylib.dylib mylib.c
./a.out

The output looks like this:

% make
echo 'int foo() { return 5; }' > mylib.c
echo "int foo() attribute((weak_import)); int printf(const char *, ...); int main() { if (foo) printf (\"have foo() %d\\\n\", foo()); else printf(\"do not have foo\\\n\"); return 0; }" > main.c
clang -dynamiclib -o libmylib.dylib mylib.c
clang main.c -L. -lmylib
./a.out
have foo() 5
echo 'int bar() { return 5; }' > mylib.c
clang -dynamiclib -o libmylib.dylib mylib.c
./a.out
do not have foo

vsk added a comment.Dec 4 2017, 4:27 PM

Ah, sorry, I should have mentioned that the header vending compression_decode_buffer() does not provide the 'weak_import' attribute (as far as I can see!). If you change your example to drop 'weak_import' from foo, I think it will be closer to the current situation.

jasonmolenda accepted this revision.Dec 4 2017, 4:29 PM

Ah I see, I bet I assumed it was marked weak but it never was. patch looks good to me.

This revision is now accepted and ready to land.Dec 4 2017, 4:29 PM
This revision was automatically updated to reflect the committed changes.