On Wed, Jul 19, 2017 at 8:19 PM, Alexander Monakov <amona...@ispras.ru> wrote:
> On Wed, 19 Jul 2017, Yuri Gribov wrote:
>> So to reiterate, your logic here is that someone would wipe dlsym type
>> (e.g. by casting to void *), then later cast to another type which
>> lacks tailcall attribute. So proposed solution won't protect against
>> situation like this.
>
> No, it's not "my logic" per se. As you said, we've arrived to this
> situation after Glibc people raised concerns about solutions akin to
>
> #define dlsym(mod, sym) ({ \
> void *__r = dlsym (mod, sym); \
> __asm__ ("" : "+g" (__r)); \
> __r; })
Actually this was a contrived example invented by Jakub to illustrate
how we can never fully prevent tailcalls :)
> not suppressing tailcalls in situations where dlsym is called via a pointer.
> I'm saying that it's not possible to solve that entirely, because the
> user will always be able to lose the attribute. It is simply not possible
> to enforce that dlsym is never tailcalled.
>
> Compare how people are always able to workaround warn_unused_result.
Sure, the best we can target for is preventing it in (hopefully vast)
majority of cases (speaking of which, I didn't see any really
problematic cases of "&dlsym" in
https://codesearch.debian.net/search?q=%26dlsym).
>> But isn't it similar to a situation when someone casts unaligned int *
>> (e.g. from a packed struct) to void * and then casts back to (aligned)
>> int *, expecting it to work (rather than SIGBUSing)? If user dumps
>> important semantic attribute, it's his responsibility to properly
>> restore it before using the object. In this particular case, he'd
>> need to cast void * to void (*)(void) __attribute__((notailcall));
>
> ... which won't work in any compiler except bleeding-edge GCC, unlike
> the asm trick that works rather broadly. If you're saying that the
> user will need to change their code, won't they prefer to change it
> in a fashion that is compatible with a wider range of compilers?
Note that with inline asm trick users will not be aware of the problem
to begin with. So if we go for it, we completely trade away detection
of tailcalls in function pointers (not just the extreme ones mentioned
above) and this was something Glibc people were specifically
interested in (https://sourceware.org/ml/libc-alpha/2017-01/msg00482.html).
Thanks for bringing up the issue of convenience. I agree that it's
important (may make sense to check this with Clang community). We
could change no_tail_call attribute to bind to declarations, rather
than types (as Jeff suggested). But this makes it pretty much
equivalent to asm trick (or __builtin_notailcall), with all its pros
and cons.
>> > Honestly, if the goal is to assist users of GCC (and not only Glibc users,
>> > but also people compiling against Bionic/musl/BSD libcs, which probably
>> > wouldn't adopt this attribute),
>>
>> Could you give more details here - why would they have problems with
>> notailcall approach?
>
> Obviously I can't speak authoritatively here, but I'm thinking that Bionic and
> BSD people won't be very interested in GCC extensions that Clang doesn't
> support, and musl generally aims to avoid using compiler extensions unless
> absolutely required (and here adding the attribute doesn't solve the problem
> in all cases anyway).
Agreed. I was tempted to say that this applies to any compiler
extension (including __builtin_notailcall) but it's not the case.
Notailcall attribute in its present form requires changes to programs
which take address of dlsym (in contrast to __builtin_notailcall).
>> > may I suggest that a clearer course of
>> > action would be to:
>> >
>> > 1) recognize dlsym by name and suppress tailcalls to it
>> >
>> > this would solve >99% cases because calling dlsym by pointer would be
>> > rare,
>> > and has the benefit of not requiring libc header changes;
>>
>> I'm not sure how this is going to save above problem with casting to void *.
>
> It wouldn't, and that's exactly what I said - it would solve direct calls, and
> indirect calls are rare and not 100% solvable in the first place.
>
>> > and
>> > 2) if we really want to offer some generic solution, can we give the users
>> > a way to suppress tailcalls via a builtin? That is, introduce
>> >
>> > foo = __builtin_notailcall (func (args))
>> > I think this would allow libc headers to do
>> >
>> > #define dlsym(sym, mod) __builtin_notailcall (dlsym (sym, mod))
>>
>> Could be done but what is the advantage compared to attribute? It
>> does not seem to fix issue you posted in the beginning.
>
> One advantage is that the builtin is more fine-grained, you can use it to
> suppress at individual call sites, so you can easily express the attribute
> in terms of the builtin using a macro as shown above or an inline function,
> but not the other way around.
I'm not sure why fine-grainedness would be important for fixing dlsym
and friends. They should simply never be tailcalled.
> Of course it doesn't do the impossible, but the point was that if we want
> to add something to the compiler, we should consider if there are more
> generic/useful alternatives.
>
>> The one and only advantage of attribute compared to Jakubs approach
>> (or yours, they share the same idea of wrapping dlsym calls) is that
>> it forces user to carry it around when taking address of function.
>
> It's an inconvenience. People won't like it when their code suddenly
> stops compiling after they update libc headers. And what's the solution
> for them? Adding GCC-8-specific attributes in their code, even if it
> never had dlsym in tail position anyway?
Right, that's reiteration of above.
So I think net conclusion is that even though typed attribute detects
more problems with dlsym-like functions, it will potentially require
modification of users code in really ugly ways (#if defined __GLIBC__
&& __GLIBC_MINOR > 28 && ...). Attribute can be relaxed but this
makes it more or less equivalent to plain and simple asm trick
proposed by Jakub. Your opinion then is that additional benefits of
typed attribute do not justify the burden.
I tend to agree, even though I'd be interested how much problematic
code is out there. Any idea how to grep it?
-Y
