[PATCH] D23674: Do not set __guard_local to hidden for OpenBSD SSP

[Stefan Kempf via llvm-commits]

Tim Shen wrote:
> On Sun, Aug 21, 2016 at 7:18 AM Stefan Kempf <sisnkemp at gmail.com> wrote:
> 
> > Tim Shen wrote:
> > > On Fri, Aug 19, 2016 at 10:28 AM Stefan Kempf <sisnkemp at gmail.com>
> > wrote:
> > >
> > > > If M.getOrInsertGlobal() does not return with a GlobalVariable,
> > > > __guard_local was declared with a different type than long. This
> > > > is a programming mistake. I decided against the assert here
> > > > because the linker should catch this, and I wanted to avoid triggering
> > > > asserts because of programming errors.
> > > >
> > >
> > > Can you elaborate on "programming mistake"?
> >
> > __guard_local is a reserved identifier and is supposed to be defined as
> > long in the startup code or the kernel only. But clang and OpenBSD's gcc
> > will happily compile a piece of source code that violates this, e.g. by
> > declaring __guard_local as char, or float (=> "programming mistake").
> >
> 
> A normal piece of source code doesn't declare __guard_local, does it? The
> user only smash a button called "-fstack-protector" on a normal piece of
> code, and the compiler should do the right thing, including generating
> `long __guard_local` declaration for the current compilation unit, then
> generate SSP code, etc.

Right. Normal code could does/should not declare __guard_local. But the
compiler lets you do so (though it's pointless for
normal code).
 
> If by calling Module::getOrInsertGlobal() inserts a `i8** __guard_local`,
> then it's llvm's fault by mistaking the return type.
> 
> I wonder if we can do:
> if (IsOpenBSD) {
>   // Insert the right declaration.
>   M.getOrInsertGlobal("__guard_local", CLongType);
>   // A lazy way to say "cast".
>   return M.getOrInsertGlobal("__guard_local", i8Ptr);
> } else {
>   // do the normal thing
> }
> 
> But I honestly don't know how to get "CLongType" in LLVM. Maybe Clang
> should generate such a declaration. Anyways, this patch doesn't make
> anything worse, so I'm happy to accept it.

long has the same width as a pointer on OpenBSD. Maybe LLVM's getIntPtrTy
could be used. But it's going to be casted to the int8ptr type for
the stack protector anyway. So it does not gain us much.
 
> Also, can you point me the place GCC implements this? I searched for
> "__guard_local" but didn't find anything.

It probably is in the OpenBSD tree only. This should create the
__guard_local decl:
http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/gnu/gcc/gcc/targhooks.c?rev=1.4&content-type=text/x-cvsweb-markup

And here the stack protector prologues/epilogues seem to be generated
(stack_protect_prologue and stack_protect_epilogue):
http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/gnu/gcc/gcc/function.c?rev=1.1&content-type=text/x-cvsweb-markup
 
> For such a program, M.getOrInsertGlobal will return a cast operation
> > instead of a GlobalValue.
> >
> > My understanding was that asserts are used to check for internal
> > compiler errors. If something is wrong in the input source code,
> > normal warnings or errors should be emitted.
> >
> > Currently the linker stage prevents that an incorrectly redefined
> > __guard_local is used in the stack protector. Maybe clang could
> > check this in the future. But I think for the moment we can get away
> > without additional diagnostics here, and save this for a separate diff.
> >