Developer forums (C::B DEVELOPMENT STRICTLY!) > Development
LINUX: Tools Run Crash and Company...
moloh:
--- Quote from: Ceniza on June 02, 2006, 07:04:31 am ---I just checked the value of this on both ToolsManager::Execute and ToolsManager::OnTerminate and it's the same, just as it should be.
The cast in there, for what it's used, makes no harm at all. The whole idea of it is that when the PipedProcess is notified about tool termination the pointer ToolsManager::m_pProcess is modified from PipedProcess to indicate just that. Since sizeof(void **) == sizeof(PipedProcess **) and there's no need to adjust offsets (what you might be wondering about that cast), it'll work. In fact that would also work for any kind of pointee (including built-in types), not just PipedProcess.
--- End quote ---
It makes, read article. There is no guarantee that it will work correctly, downcasting this value when there is multiple inheritance is compiler dependent.
On Gentoo it sometimes makes crashes (gcc-4.1) or on older compiler (gcc-3.4) it works ok, confirmed by few people i contacted, also on debian by myself (i only check crashes).
You can also check other posts in this thread, crashes are confirmed there by other developers. They use linux, valgrind will show them still invelid reads with this pointer as in my case...
--- Quote from: Ceniza on June 02, 2006, 07:04:31 am ---Now, the purpose of m_pProcess is to know if there's a tool running, just that. If you try to run another tool and m_pProcess is not 0, you'll get a message saying there's another tool already running. It's just being sure the "process terminated setting that pointer to 0" trick is applied as soon as the tool finishes. It'll also create a new event which will get ToolsManager::OnTerminate executed, which BTW sets the pointer once again to 0. This suggests the cast could be replaced by a plain 0 (a NULL pointer) and it should work.
If you check what's that pointer being used for in PipedProcess, you'll find it's just used to set to 0 the pointer it's pointing to. It'sn't casted to anything else and then trying to call a member function of the resulting pointer.
That said, keeping it that way, it'll do what it's intended to do.
If you really dislike that C-cast (I do), it could, at most, be replaced by a reinterpret_cast, even though unnecessary.
--- End quote ---
It wont work, read article why. I belive in what is written there as i saw results with my bare eyes in Code::Blocks.
Also i understand how this thing work, but for the first time i saw this kind of construction (that dynamic object set self pointer in "owner" class). For me this was strage, why do not modify m_pProcess pointer only in event ToolsManager class, now it first is set to 0 by PipedProcess class, and then in ToolsManager OnToolTerminated event handler.
Ceniza:
Here you have a reinterpret cast with multiple inheritance:
--- Code: (asm) ---leal -28(%ebp), %eax
movl %eax, -32(%ebp)
--- End code ---
In words: get the address of a variable (which is a pointer) and store it in another variable (which is a pointer to pointer).
Once again: it is NOT a downcast, it IS a reinterpret cast, and, again, that's the intention.
Modifying the pointer only in ToolsManager is a choice, but really, don't get yourself confused. That would save us a double assignment to that pointer which happens to be two asm lines (because of the indirection), and just that.
--- Code: (asm) ---movl -32(%ebp), %eax
movl $0, (%eax)
--- End code ---
Just for fun. Here you have the code of a downcast with offset adjustment (4 bytes for this test) and which also checks for NULL (well, it really has to):
--- Code: (asm) --- cmpl $0, -28(%ebp)
je L2
movl -28(%ebp), %eax
addl $4, %eax
movl %eax, -36(%ebp)
jmp L3
L2:
movl $0, -36(%ebp)
L3:
movl -36(%ebp), %eax
movl %eax, -32(%ebp)
--- End code ---
Anything else to prove?
moloh:
Ok here You got me. I can't speak in assembler language. But this way it has to work, because You do work of compiler.
Do You want me to say You are better? Here You go... You are better.
To show You the problem i will prepare a build and send binary and diff of sources to You, with screenshots (in ~24h).
One thing, i am very suprised how this work, i never thought that c pointer cast can change address, but You will see that too.
Ceniza:
Just give me something I can test, like: Add tool with name xxx that calls yyy with parameters zzz and blah blah blah, check the value of this here and there... something like that.
Just be sure to include filename and line :)
[edit]
I just took a closer look to the line that surprised you. If you take a closer look too you'll find it's OK :)
--- Code: (cpp) ---m_pProcess =
new PipedProcess((void**)
&m_pProcess, // address of m_pProcess != m_pProcess
this, idToolProcess, pipe, dir);
--- End code ---
[/edit]
moloh:
--- Quote from: Ceniza on June 02, 2006, 08:42:47 am ---Just give me something I can test, like: Add tool with name xxx that calls yyy with parameters zzz and blah blah blah, check the value of this here and there... something like that.
--- End quote ---
Already did this here:
--- Quote from: moloh on May 23, 2006, 11:22:07 am ---Added tool named: ln
Executable: ln
Parameters: --help
--- End quote ---
--- Quote from: Ceniza on June 02, 2006, 08:42:47 am ---Just be sure to include filename and line :)
--- End quote ---
Diff of changes:
--- Code: ---diff -ur codeblocks/src/sdk/toolsmanager.cpp codeblocks.my/src/sdk/toolsmanager.cpp
--- codeblocks/src/sdk/toolsmanager.cpp 2006-05-27 16:42:42.000000000 -0600
+++ codeblocks.my/src/sdk/toolsmanager.cpp 2006-06-02 00:35:23.000000000 -0600
@@ -159,6 +159,12 @@
break;
}
+ Manager::Get()->GetMessageManager()->Log(_T("This in Execute: %d"), this);
+ Manager::Get()->GetMessageManager()->Log(_T("This in Execute (c cast): %d"), (wxEvtHandler *)this);
+ Manager::Get()->GetMessageManager()->Log(_T("This in Execute (static cast): %d"), static_cast<wxEvtHandler *>(this));
+ Manager::Get()->GetMessageManager()->Log(_T("This in Execute (reinterpret cast): %d"), reinterpret_cast<wxEvtHandler *>(this));
+ Manager::Get()->GetMessageManager()->Log(_T("This in Execute (dynamic cast): %d"), dynamic_cast<wxEvtHandler *>(this));
+
m_pProcess = new PipedProcess((void**)&m_pProcess, this, idToolProcess, pipe, dir);
m_Pid = wxExecute(cmdline, flags, m_pProcess);
@@ -392,6 +398,8 @@
void ToolsManager::OnToolTerminated(CodeBlocksEvent& event)
{
+ Manager::Get()->GetMessageManager()->Log(_T("This in OnToolTerminated: %d"), this);
+
m_Pid = 0;
m_pProcess = 0;
--- End code ---
Where to send binary (17MB, tar.gz)?
--- Quote from: Ceniza on June 02, 2006, 08:42:47 am ---[edit]
I just took a closer look to the line that surprised you. If you take a closer look too you'll find it's OK :)
--- Code: (cpp) ---m_pProcess =
new PipedProcess((void**)
&m_pProcess, // address of m_pProcess != m_pProcess
this, idToolProcess, pipe, dir);
--- End code ---
[/edit]
--- End quote ---
I all the time speak about *this* pointer in this call.
Screenshot attached. I must say i am suprised about reinterpret_cast, it works, i must eveluate why there is change there...
[edit]
After few minutes and some reading, now i am not suprised why reinterpret_cast works as in screenshot ;-)
[/edit]
[attachment deleted by admin]
Navigation
[0] Message Index
[#] Next page
[*] Previous page
Go to full version