Re: [user32] Add DECLSPEC_NOINLINE, use it on call_hook_proc()

[Qian Hong]

Hi Dan,

Thanks for working on it! It is really an annoying bug, however, is
adding DECLSPEC_NOINLINE a workaround or a real fix? If it is only a
workaround, would that hide some real bug behind it? Or is that
possible that there is a gcc bug here that we want to report to gcc?

Thanks for your time!

[user32] Add DECLSPEC_NOINLINE, use it on call_hook_proc()

[Dan Kegel]

I suspect this is a real fix, and there is no gcc bug.

On Fri, Apr 19, 2013 at 9:47 AM, Qian Hong  wrote:
> Hi Dan,
>
> Thanks for working on it! It is really an annoying bug, however, is
> adding DECLSPEC_NOINLINE a workaround or a real fix? If it is only a
> workaround, would that hide some real bug behind it? Or is that
> possible that there is a gcc bug here that we want to report to gcc?
>
> Thanks for your time!

[user32] Add DECLSPEC_NOINLINE, use it on call_hook_proc()

[Qian Hong]

On Sat, Apr 20, 2013 at 12:52 AM, Dan Kegel  wrote:
> I suspect this is a real fix, and there is no gcc bug.

Thanks for clarify.

Curiosity killed the cat, what is the theory behind this patch?
I tried explicitly add 'inline' to every static functions in hook.c
but complie with -O0, to see if the bug can be reproduced in this way,
but nothing happen, this make me doubt being inline is not the
culprit.

Any inspire is great appreciated!

Best wishes from a curiosity cat :)

--
Regards,
Qian Hong

-
http://www.winehq.org

[user32] Add DECLSPEC_NOINLINE, use it on call_hook_proc()

[André Hentschel]

Am 19.04.2013 19:13, schrieb Qian Hong:
> On Sat, Apr 20, 2013 at 12:52 AM, Dan Kegel  wrote:
> > I suspect this is a real fix, and there is no gcc bug.
>
> Thanks for clarify.
>
> Curiosity killed the cat, what is the theory behind this patch?
> I tried explicitly add 'inline' to every static functions in hook.c
> but complie with -O0, to see if the bug can be reproduced in this way,
> but nothing happen, this make me doubt being inline is not the
> culprit.
>
> Any inspire is great appreciated!
>
> Best wishes from a curiosity cat :)

the inline keyword is meanwhile just a hint for the compiler, it doesn't need to inline it

[user32] Add DECLSPEC_NOINLINE, use it on call_hook_proc()

[Dan Kegel]

On Fri, Apr 19, 2013 at 10:13 AM, Qian Hong  wrote:
> Curiosity killed the cat, what is the theory behind this patch?

Hooking is a fragile business.  Somebody somewhere is probably
making assumptions about how hooking works (like, how many stack
frames are pushed), and inlining call_hook_proc probably violates one
of those assumptions.
> I tried explicitly add 'inline' to every static functions in hook.c
> but complie with -O0, to see if the bug can be reproduced in this way,
> but nothing happen, this make me doubt being inline is not the
> culprit.

call_hook_proc is probably the only function whose inlining matters
for this problem (since my patch solved your problem at -O2).

(inline is only a hint. FORCEINLINE is stronger.)
- Dan

[user32] Add DECLSPEC_NOINLINE, use it on call_hook_proc()

[Dan Kegel]

On Fri, Apr 19, 2013 at 10:38 AM, Dan Kegel  wrote:
> Hooking is a fragile business.  Somebody somewhere is probably
> making assumptions about how hooking works (like, how many stack
> frames are pushed), and inlining call_hook_proc probably violates one
> of those assumptions.

That said, this is all a big guess.  It would be nice to have a
test that demonstrated the problem.  It's up to Alexandre to
decide whether making a couple apps happier is evidence
enough that disabling inlining of this one function is a good idea.
- Dan

[user32] Add DECLSPEC_NOINLINE, use it on call_hook_proc()

[Alexandre Julliard]

Dan Kegel  writes:
> On Fri, Apr 19, 2013 at 10:38 AM, Dan Kegel  wrote:
> > Hooking is a fragile business.  Somebody somewhere is probably
> > making assumptions about how hooking works (like, how many stack
> > frames are pushed), and inlining call_hook_proc probably violates one
> > of those assumptions.
>
> That said, this is all a big guess.  It would be nice to have a
> test that demonstrated the problem.  It's up to Alexandre to
> decide whether making a couple apps happier is evidence
> enough that disabling inlining of this one function is a good idea.

It's not. Please take the time to understand the problem and fix it
properly.

[user32] Add DECLSPEC_NOINLINE, use it on call_hook_proc()

[Qian Hong]

On Sat, Apr 20, 2013 at 1:38 AM, Dan Kegel  wrote:
> > I tried explicitly add 'inline' to every static functions in hook.c
> > but complie with -O0, to see if the bug can be reproduced in this way,
> > but nothing happen, this make me doubt being inline is not the
> > culprit.
>
> call_hook_proc is probably the only function whose inlining matters
> for this problem (since my patch solved your problem at -O2).
>
> (inline is only a hint. FORCEINLINE is stronger.)

Thanks Dan and André, I tried FORCEINLINE and tried bisect on gcc
flags to reproduce the bug but failed with strange gcc behaviors, then
Dan had worked out an actual fix for this bug, what I learned is,
understanding the real bug seems faster than playing with gcc flags...

Anyway, thanks Dan and André as always!

--
Regards,
Qian Hong

-
http://www.winehq.org