Please attach preprocessed source.

Created attachment 11030 [details]
Preprocessed test case

The output of the following command:
clang++ -D_FORTIFY_SOURCE=2 -O2 -stdlib=libstdc++ -E -o test.preprocessed test.cpp

Created attachment 11031 [details]
LLVM representation of the test case

The output of the following command:
clang++ -D_FORTIFY_SOURCE=2 -O2 -stdlib=libstdc++ -S -emit-llvm -o test.llvm-rep test.cpp

Created attachment 11032 [details]
Assembly output for the test case

The output of the following command:
clang++ -D_FORTIFY_SOURCE=2 -O2 -stdlib=libstdc++ -S -o test.compiled test.cpp

FWIW, the same problem persists when using libc++.

Rafael, this is probably yours; it's related to the CodeGenModule::isTriviallyRecursive hack.

(In reply to comment #6)
> Rafael, this is probably yours; it's related to the
> CodeGenModule::isTriviallyRecursive hack.

I think this is a case we are just not able to fully represent in the IL currently. The preprocessed source has what looks like

void bar(void);
extern void __foo_alias () __asm__ ("foo");
extern __inline void foo()
{
  bar();
  return __foo_alias ();
}
void *p;
void zed() {
  foo();
  p = foo;
}

where bar in here represents the checks added by -D D_FORTIFY_SOURCE=2.

The hack we have in clang tries to see these "fake recursive" functions and call the external one directly. I think it is not kicking in because of a lack of -fgnu89-inline in the command line, but that is not what we want in here as doing that will drop the checks the user is trying to add.


What gcc 4.8 -O0 produces in the above example is

        call    foo
        movq    $foo, p(%rip)

When optimizing a call to bar shows up, showing that the inline version of foo was used.

What I think we need is an IL like

declare void @bar()
declare void @foo()
define available_externally void @foo_inline() {
  call void @bar()
  call void @foo()
  ret void
}
@p = common global i8* null, align 8
define void @zed() {
  call void @foo_inline()
  store i8* bitcast (void ()* @foo to i8*), i8** @p, align 8
  ret void
}

the logic being:

* the declaration of  __foo_alias produces foo in the llvm IR because of the ASM.
* the definition of an gnu inline foo in C when a declaration of foo is already present (because of the ASM) causes clang to emit an available_externally definition with an unique name (foo_inline in the above example).
* calls to __foo_alias in C produce calls to foo in the IR.
* call to foo in C produce calls to foo_inline in the IR.
* Other user of foo in C use foo in the IR.

This would work for the original testcase since the inline function is marked always_inline, but in general we need something like

define available_externally void @foo_inline() external @foo {

to tell codegen to produce a reference to foo if foo_inline is not inlined.

(In reply to comment #7)
> The hack we have in clang tries to see these "fake recursive" functions and
> call the external one directly. I think it is not kicking in because of a
> lack of -fgnu89-inline in the command line,

This is C++; -fgnu89-inline wouldn't have any effect.

(In reply to comment #8)
> (In reply to comment #7)
> > The hack we have in clang tries to see these "fake recursive" functions and
> > call the external one directly. I think it is not kicking in because of a
> > lack of -fgnu89-inline in the command line,
> 
> This is C++; -fgnu89-inline wouldn't have any effect.

The glibc source code has:

#  define __extern_always_inline \
  extern __always_inline __attribute__ ((__gnu_inline__))

...

#define __fortify_function __extern_always_inline __attribute_artificial__


but I guess the glibc being used in this test is older or clang is not getting there because of ifdefs (there is no gnu_inline in the testcase).

With c++ semantics this glibc trick becomes *really* odd, since we get a linkonce_odr function in on TU that is different from a strong function in another TU.

*** Bug 17054 has been marked as a duplicate of this bug. ***

When will this bug get fixed?
(In reply to comment #10)
> *** Bug 17054 has been marked as a duplicate of this bug. ***

I have read the code.And actually thinking clang is the one generating the right code.

*** Bug 18775 has been marked as a duplicate of this bug. ***

So, is this a bug in the fresh libc headers or in clang? 
If in clang, any hope to fix? 
This is blocking the build of Chromium on Ubuntu 13.10 and newer.

(In reply to comment #14)
> So, is this a bug in the fresh libc headers or in clang? 
> If in clang, any hope to fix? 
> This is blocking the build of Chromium on Ubuntu 13.10 and newer.

I would call it a messing feature in clang. I have changed my opinion on how to implement this. I think that given

void bar(void);
extern void __foo_alias () __asm__ ("foo");
extern __inline void foo()
{
  bar();
  return __foo_alias ();
}
void *p;
void zed() {
  foo();
  p = foo;
}


clang should produce

declare void @bar()
declare void @foo()
define internal void @foo_inline() {
  call void @bar()
  call void @foo()
  ret void
}
@p = common global i8* null, align 8
define void @zed() {
  call void @foo_inline()
  store i8* bitcast (void ()* @foo_inline to i8*), i8** @p, align 8
  ret void
}

This is not exactly the same thing that gcc produces: gcc will simply not use the wrapper if it is not a direct call or if it is not inlined. Hopefully all users will consider this to be a bug and not a feature of how this implemented in gcc.

The advantages are:
* in the above example bar is always called before foo
* we don't need any new features in llvm

An other test case:

#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>

int main ()
{
    char *data;
    int  pagesize;
    int fd, foo;
    pagesize = getpagesize ();
    fd = open ("conftest.mmap", O_RDWR);
    data = (char *) malloc (pagesize);
    foo = read (fd, data, pagesize);
}

clang++ -o conftest -g  -O2  -D_FORTIFY_SOURCE=2 conftest.cpp

In the asm generated, clang will replace the call by __read_chk while
g++ uses read.

Am I right this is a Linux-only bug because OSX uses different system headers?

Yes, I don't think Mac OS X has fortify sources

It actually does, there's a _FORTIFY_SOURCE definition in _types.h that controls _USE_FORTIFY_LEVEL. But the implementation seems to differ, so I wonder whether the issue in question applies to OSX.

OK. My bad.

I just tested on my Debian Jessie (currently testing) and I cannot reproduce my test case...

We also found a problem with this on CentOS7 and clang++ 3.4.2. Below is the minimal test case where we can see that the __readlink_chk function isn't called at all and is instead replaced by the infinite loop.

#include <unistd.h>

int main() {
  auto path_max = pathconf(0, _PC_PATH_MAX);
  auto result_heap = (char*)malloc(path_max);
  auto size = readlink(0, result_heap, path_max);
}

$ /usr/bin/clang++ -O1 -D_FORTIFY_SOURCE=1 -o test -std=c++11 test.cpp

$ gdb -batch -ex 'file ./test' -ex 'disassemble main'
Dump of assembler code for function main:
   0x0000000000400640 <+0>:     push   %rax
   0x0000000000400641 <+1>:     xor    %edi,%edi
   0x0000000000400643 <+3>:     mov    $0x4,%esi
   0x0000000000400648 <+8>:     callq  0x400540 <pathconf@plt>
   0x000000000040064d <+13>:    mov    %rax,%rdi
   0x0000000000400650 <+16>:    callq  0x400520 <malloc@plt>
   0x0000000000400655 <+21>:    data32 nopw %cs:0x0(%rax,%rax,1)
   0x0000000000400660 <+32>:    jmp    0x400660 <main+32>
End of assembler dump.