On Mon, Nov 16, 2020 at 7:48 AM Ian Rogers <irog...@google.com> wrote:
>
> GCC [0-9]+ :-) Perhaps just a reference to the GCC bug rather than a date.
That would be very good.
> In linux/compiler_attributes.h add:
> #define __GCC4_has_attribute_disable_tail_calls 0
> to the #ifndef __has_attribute block. We can't do this locally here as after
> that #include __has_attribute will be defined.
As far as I know, `tools/` use their own `compiler*` files, which is
why I was suggesting creating the equivalent there.
> In terms of lines of code, there's not much difference. Arguably there is a
> bit more cognitive load from the #include and that disable_tail_call needs
> the funny handling that's here but won't obviously be hinted at by placing it
> in a shared header. I'm a little concerned that someone will come across this
> in shared code and then go and break this test again with well intentioned
> cleanup.
Fewer lines, fewer conditions :-) The `#include` is hardly important
given kernel developers already know and use compiler attributes in
many places (they are included in the majority of compilation units).
Actually, we can simplify further. The attribute itself should be
pulled from the `compiler_attributes.h` (a `tools/` one, if needed),
and the barrier should likely be the `barrier()` macro (ditto).
Then, we just need:
#if __has_attribute(disable_tail_calls)
# define NO_TAIL_CALL_BARRIER
#else
# define NO_TAIL_CALL_BARRIER barrier()
#endif
because using the attribute directly just works -- the only special
thing here is the conditional barrier.
And this conditional barrier should probably be shared, too, defining
it wherever `barrier()` (or equivalent) is defined for `tools/`. And
the name could be `barrier_for_tail_call()` or something like that.
Of course, we don't need to do all this for this patch, but we should
always attempt to minimize/simplify the diffs later on -- that is why
I suggested using the unconditional `__has_attribute` as if it was
already properly defined (we had to untangle similar stuff when I
added `compiler_attributes.h`).
Cheers,
Miguel
