Skip to content

clang does not know fno-delete-null-pointer-checks#664

Merged
mehlis merged 1 commit intoRIOT-OS:masterfrom
Kijewski:conditional-fno-delete-null-pointer-checks
Feb 12, 2014
Merged

clang does not know fno-delete-null-pointer-checks#664
mehlis merged 1 commit intoRIOT-OS:masterfrom
Kijewski:conditional-fno-delete-null-pointer-checks

Conversation

@Kijewski
Copy link
Copy Markdown
Contributor

@Kijewski Kijewski commented Feb 8, 2014

as introduced in #628 the parameter "-fno-delete-null-pointer-checks" is handed over to the compiler.

clang throws a warning and ignores it.

task: try to find a way to get the desired functionality in clang, and or try to find a way to get rid of this warning.

@Kijewski
Copy link
Copy Markdown
Contributor

Kijewski commented Feb 8, 2014

See clang bug 9251 - Support -fno-delete-null-pointer-checks. The feature request is staled since 2011-12-02.

But clang does not seem to use the delete-null-pointer-checks optimization, so it's safe to omit the argument. But I'm not sure how to probe whether the supplied $(CC) supports -fno-delete-null-pointer-checks.

Test input:

#include <stdio.h>

int main(void)
{
    unsigned *volatile x_ = NULL;

    unsigned *x = x_;
    printf("%u\n", *x);
    if (x) {
        return 0;
    }
    return 1;
}

gcc:

08048300 <main>:
 8048300:       55                      push   %ebp
 8048301:       89 e5                   mov    %esp,%ebp
 8048303:       83 e4 f0                and    $0xfffffff0,%esp
 8048306:       83 ec 20                sub    $0x20,%esp
 8048309:       c7 44 24 1c 00 00 00    movl   $0x0,0x1c(%esp)
 8048310:       00 
 8048311:       8b 44 24 1c             mov    0x1c(%esp),%eax
 8048315:       8b 00                   mov    (%eax),%eax
 8048317:       c7 04 24 b0 84 04 08    movl   $0x80484b0,(%esp)
 804831e:       89 44 24 04             mov    %eax,0x4(%esp)
 8048322:       e8 a9 ff ff ff          call   80482d0 <printf@plt>
 8048327:       31 c0                   xor    %eax,%eax  // set result to zero w/o any check
 8048329:       c9                      leave  
 804832a:       c3                      ret

clang:

08048420 <main>:
 8048420:       56                      push   %esi
 8048421:       83 ec 18                sub    $0x18,%esp
 8048424:       c7 44 24 14 00 00 00    movl   $0x0,0x14(%esp)
 804842b:       00 
 804842c:       8b 74 24 14             mov    0x14(%esp),%esi
 8048430:       8b 06                   mov    (%esi),%eax
 8048432:       89 44 24 04             mov    %eax,0x4(%esp)
 8048436:       c7 04 24 e0 84 04 08    movl   $0x80484e0,(%esp)
 804843d:       e8 ae fe ff ff          call   80482f0 <printf@plt>
 8048442:       85 f6                   test   %esi,%esi // ← test if x == NULL
 8048444:       0f 94 c0                sete   %al       // ← set result accordingly
 8048447:       0f b6 c0                movzbl %al,%eax
 804844a:       83 c4 18                add    $0x18,%esp
 804844d:       5e                      pop    %esi
 804844e:       c3                      ret

@mehlis
Copy link
Copy Markdown
Contributor Author

mehlis commented Feb 8, 2014

nice analyses, so we get useful results with clang without having this switch implemented in clang

@Kijewski
Copy link
Copy Markdown
Contributor

Kijewski commented Feb 8, 2014

I wrapped the CFLAGS += -fno-delete-null-pointer-checks in an if block, testing whether the compiler understands the flag. Seems to work.
Please confirm.

Makefile.include Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please write two lines:

  1. why this switch is needed (gcc)
  2. why it is not enabled in all cases (clang)

@Kijewski
Copy link
Copy Markdown
Contributor

Kijewski commented Feb 9, 2014

Would a reference to #628 and #664 be enough?

@mehlis
Copy link
Copy Markdown
Contributor Author

mehlis commented Feb 9, 2014

@Kijewski you can copy the content of the issues, just one sentence: what is the motivation for the switch, what is the reason to not enable it in every case.:) I think it's best to have a short version directly in the file, and a link to the discussion is always a benefit.

@mehlis
Copy link
Copy Markdown
Contributor Author

mehlis commented Feb 10, 2014

$ BOARD=native LANG=C CC=clang CFLAGS=" -DHAVE_VALGRIND_VALGRIND_H -O0 -g -Wall -Wextra -pedantic" make -B clean all

seems not to work with that command line...

@Kijewski
Copy link
Copy Markdown
Contributor

hello-world and default work for me with that command line.
You might want to elaborate.

@mehlis
Copy link
Copy Markdown
Contributor Author

mehlis commented Feb 10, 2014

I'm on current master with this commit cherry picked in examples/default.
I enter:
$ BOARD=native LANG=C CC=clang CFLAGS=" -DHAVE_VALGRIND_VALGRIND_H -O0 -g -Wall -Wextra -pedantic" make -B all

and I get:

Compiling.... main.c

clang: warning: argument unused during compilation: '-fno-delete-null-pointer-checks'
Building project default for native w/ MCU native.
"make" -C /home/c/git/RIOT-OS/RIOT/boards/native
make[1]: Entering directory `/home/c/git/RIOT-OS/RIOT/boards/native'
clang: warning: argument unused during compilation: '-fno-delete-null-pointer-checks'

Only use -fno-delete-null-pointer-checks if the supplied compiler knows
the flag. Clang does not understand the flag, and does not need it.
@Kijewski
Copy link
Copy Markdown
Contributor

You have an old clang version. apt-get dist-upgrade ;) Since v3.4 clang fails now if an unknown flag was supplied.

I amended my commit to check if the output on STDERR was empty when testing if the flag is known.

@mehlis
Copy link
Copy Markdown
Contributor Author

mehlis commented Feb 10, 2014

Ah I see. I'm on oldstable fedora. going to test this tomorrow with stable.

@mehlis
Copy link
Copy Markdown
Contributor Author

mehlis commented Feb 12, 2014

tested with clang 3.4. it works. ACK.

can someone else test this too?

@LudwigKnuepfer
Copy link
Copy Markdown
Member

Successfully tested that building yields no warnings with:

clang -v
Apple clang version 4.0 (tags/Apple/clang-421.0.60) (based on LLVM 3.1svn)
Target: x86_64-apple-darwin12.4.0
Thread model: posix

gcc -v
Using built-in specs.
Target: i686-apple-darwin11
Configured with: /private/var/tmp/llvmgcc42/llvmgcc42-2336.11~28/src/configure --disable-checking --enable-werror --prefix=/Applications/Xcode.app/Contents/Developer/usr/llvm-gcc-4.2 --mandir=/share/man --enable-languages=c,objc,c++,obj-c++ --program-prefix=llvm- --program-transform-name=/^[cg][^.-]*$/s/$/-4.2/ --with-slibdir=/usr/lib --build=i686-apple-darwin11 --enable-llvm=/private/var/tmp/llvmgcc42/llvmgcc42-2336.11~28/dst-llvmCore/Developer/usr/local --program-prefix=i686-apple-darwin11- --host=x86_64-apple-darwin11 --target=i686-apple-darwin11 --with-gxx-include-dir=/usr/include/c++/4.2.1
Thread model: posix
gcc version 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.11.00)

and

clang -v                  
clang version 3.4 (tags/RELEASE_34/final)
Target: x86_64-unknown-linux-gnu
Thread model: posix
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.1.2
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.2
Found candidate GCC installation: /usr/bin/../lib64/gcc/x86_64-unknown-linux-gnu/4.1.2
Found candidate GCC installation: /usr/bin/../lib64/gcc/x86_64-unknown-linux-gnu/4.8.2
Found candidate GCC installation: /usr/lib/gcc/x86_64-unknown-linux-gnu/4.1.2
Found candidate GCC installation: /usr/lib/gcc/x86_64-unknown-linux-gnu/4.8.2
Found candidate GCC installation: /usr/lib64/gcc/x86_64-unknown-linux-gnu/4.1.2
Found candidate GCC installation: /usr/lib64/gcc/x86_64-unknown-linux-gnu/4.8.2
Selected GCC installation: /usr/bin/../lib64/gcc/x86_64-unknown-linux-gnu/4.8.2

mehlis pushed a commit that referenced this pull request Feb 12, 2014
…nter-checks

clang does not know fno-delete-null-pointer-checks
@mehlis mehlis merged commit 07c5ae3 into RIOT-OS:master Feb 12, 2014
kaspar030 pushed a commit to kaspar030/RIOT that referenced this pull request Feb 12, 2014
Only use -fno-delete-null-pointer-checks if the supplied compiler knows
the flag. Clang does not understand the flag, and does not need it.
kaspar030 pushed a commit to kaspar030/RIOT that referenced this pull request Apr 3, 2014
Only use -fno-delete-null-pointer-checks if the supplied compiler knows
the flag. Clang does not understand the flag, and does not need it.
kaspar030 pushed a commit to kaspar030/RIOT that referenced this pull request Apr 3, 2014
In RIOT-OS#664 I added a test that determines if the supplied compiler
understands the `-fno-delete-null-pointer-checks` flag. The problem is
that the `$(CC)` supplied on command line or in the application's
Makefile is used, but not the one the `$(BOARD)`'s Makefile sets.

That problem was overlooked as all the boards use GCC, and GCC happens
to know the flag. But if some future board does not use GCC, then the
wrong order of the checks could pose a problem.
@Kijewski Kijewski deleted the conditional-fno-delete-null-pointer-checks branch May 7, 2014 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants