On Wed, May 20, 2020 at 11:24 AM Viacheslav Usov <via.usov@gmail.com> wrote:
>
> I'd say in this
>
> if (globalhook || (L->hookmask & LUA_MASKRET)) { /* is return hook on? */
>
> and other places where you introduced a second conditional, it was not really necessary.
>
> You could define another hook flag LUA_GLOBALHOOK, and set it up in L->hookmask whenever G(L)->globalhook is set up with a non-null value, and also when switching from one Lua thread to another.
>
> You would still have to check G(L)->globalhook again, but that would happen only in the slow path.
>
> And, since I am talking about your code, your use of 'volatile' is technically incorrect, you need an atomic type instead, but I am afraid it was not you who started this in the Lua codebase.
I know relying on "volatile" is technically incorrect according to the
standard, but I think it's the least bad option here anyway for two
reasons: First, as you said, Lua already makes the assumption that
this works, so my new code won't be broken on any platforms where
regular hooks aren't already broken, and it so happens that on amd64
at least, it does work correctly. Second, using atomics would require
C11, which Lua doesn't currently require, and they're not compatible
between C and C++, so it'd be a horrible #ifdef soup of three
different versions of all of the code that uses them. Now having said
that, I think it is a neat idea to try even if it is too messy to
incorporate, so I think I will write another patch that does that.
Atomics may break arbitrarily if multithreading is used.
(Appologies for the double post, hit Send by accident). Just because it does work now does not mean that a different platform, or even a random compiler optimization would not break stuff. Data races are a really nice footgun, as they can even happen on a whim. Maybe You started the program on an odd numbered cycle, so the read order is broken. So many ways a data race that seemingly works "fine" can just arbitrarily break.
As for adding another hook flag to L->hookmask, wouldn't that then
make this no better than hooks as they stand today? In particular, it
would have the original race condition since L->hookmask isn't even
checked if ci->u.l.trap doesn't get set, and it wouldn't work to
interrupt other coroutines anymore since that's in L and not in G(L).
Joseph C. Sible
