robik / ConsoleD

Because colors are awesome.
63 stars 11 forks source link

RealTimeConsoleInput dtor doesn't seem to be working #3

Open quickfur opened 11 years ago

quickfur commented 11 years ago

I'm running into a strange problem. When running the included demo main(), after hitting Q to quit, I discovered that ECHO is still disabled on my terminal, causing the shell prompt to have invisible input. I have to run /usr/bin/reset to fix this.

I traced the problem to what I think is a compiler bug: RealTimeConsoleInput's dtor correctly calls the delegate to restore the original terminal settings saved in .old, but for some reason, when I insert writeln's into the delegate, I find that the ECHO flag in .old has mysteriously been reset to 0. Inserting the same writeln outside the delegate shows that the ECHO flag was set when tcgetattr first returned it. A brief look over the code seems to indicate that nowhere else in the code does it touch this flag, so it looks like some kind of memory corruption is happening before the dtor is called.

I know this is probably not a bug in terminal.d per se, but I'd like to know how to work around this problem, as it's currently making it impractical to use terminal.d in my own programs. :-(

adamdruppe commented 11 years ago

Here's a workaround, put the call direct instead of in a delegate

~this() { tcsetattr(fd, TCSANOW, &old); // we're just undoing everything the constructor did, in reverse order, same criteria foreach_reverse(d; destructor) d(); }

adamdruppe commented 11 years ago

i pushed a workaround, give it a try and be sure it works for you too.

Though I used the same kind of delegate thing on Windows as well, so we could have the same problem there.

quickfur commented 11 years ago

Is this a known bug in dmd?

quickfur commented 11 years ago

Tested it, seems to work. Thanks!

adamdruppe commented 11 years ago

On Fri, Jan 18, 2013 at 11:09:02AM -0800, H. S. Teoh wrote:

Is this a known bug in dmd?

I don't know... I'm not sure what exactly the bug is yet: could be struct related, or closure related, or maybe I screwed up in the code somewhere.

I guess we should see about reducing it to a smaller test.

quickfur commented 11 years ago

I determined that the bug occurs if you make a function call (to any function) after calling terminal.captureInput. It's unrelated to the loop that prints out the events. I made an empty function that I called after calling captureInput, and the bug was triggered. If main() exits immediately after calling captureInput, the bug is not triggered (or at least, it doesn't surface, it may still be there).

Looks definitely like something fishy is going on with stack-based objects somewhere.

quickfur commented 11 years ago

I've managed to minimize the code:

http://d.puremagic.com/issues/show_bug.cgi?id=9352

quickfur commented 11 years ago

Actually, I wonder if the delegate problem can be worked around by declaring .old as a local variable in the ctor (to cause the compiler to allocate it on the heap) instead of as a struct member. That should keep the value safe from invalidation by the struct copy, right?

adamdruppe commented 11 years ago

On Fri, Jan 18, 2013 at 12:28:54PM -0800, H. S. Teoh wrote:

Actually, I wonder if the delegate problem can be worked around by declaring .old as a local variable in the ctor (to cause the compiler to allocate it on the heap) instead of as a struct member. That should keep the value safe from invalidation by the struct copy, right?

it didn't work when I quickly tried it... and check this out:

void delegate() lol; import std.stdio;

struct D { int member; this(int d) { int a = 10; member = d; lol = { writeln(a, " :: ", member); }; writeln(lol.ptr, " ", &this); } }

D foo() { return D(100); }

void main() { int c; auto d = foo(); c = 20; lol(); writeln(lol.ptr, " ", &d);

}

F7410FF0 FFCA0284 10 :: 10 F7410FF0 FFCA02A0

The colon numbers should not have been the same. It should have been 10 :: 100

Looks like in this case, the delegate is pointing at the heap, where the locals were copied.

The struct instance though, is somewhere else entirely. this.member would compile to the same assembly as accessing the local (I think, I was going to check, but with writeln in there, the object file dumps too much spam)

Something like mov eax, [ebx + 4], where ebx is the data pointer. If the data pointer was the struct, we'd get the member, but since it is a copy of the stack, we get the local instead.

soooo oh my.

quickfur commented 11 years ago

Whoa. This is so many levels of wrong... but fortunately, it appears that this has been fixed in the latest git HEAD. I tested your code in my environment, and it printed 10 :: 100 correctly.