Re: [Kicad-developers] About Bug 1604841: Pcbnew crash at moving via, and boost::context fixes to make it compatible with boost 1.61

[Maciej Sumiński]

Hi Michael,

Good job, I know it was not trivial. I have just tested the patch
against the same boost versions under Linux and it works as expected.
Thank you for the fix, it has been committed in revision 7002.

Regards,
Orson

On 08/02/2016 02:26 AM, Michael Steinberg wrote:
> Hello all,
> 
> attached is v2 of the crash fix. My original patch would move the
> function out of the transition before a copy of it could be pushed to
> the state stack.
> 
> The second patch refactors coroutine, mostly for clarification and
> correctness purposes. A few documentation hints were added. It also
> removes many unnecessary heap allocations of single pointers. I compiled
> and tested it with boost 1.54, 1.60 and 1.61. Compile- and runtime-tests
> on these and other boost versions would be very welcome, this stuff is
> very brittle! I'm almost at a point having the tree duplicated for every
> boost version, for not having to recompile kicad completely when
> switching them...
> 
> Michael
> 
> 
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> 




signature.asc
Description: OpenPGP digital signature
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] About Bug 1604841: Pcbnew crash at moving via, and boost::context fixes to make it compatible with boost 1.61

[jp charras]

Le 02/08/2016 à 02:26, Michael Steinberg a écrit :
> Hello all,
> 
> attached is v2 of the crash fix. My original patch would move the function 
> out of the transition
> before a copy of it could be pushed to the state stack.
> 
> The second patch refactors coroutine, mostly for clarification and 
> correctness purposes. A few
> documentation hints were added. It also removes many unnecessary heap 
> allocations of single
> pointers. I compiled and tested it with boost 1.54, 1.60 and 1.61. Compile- 
> and runtime-tests on
> these and other boost versions would be very welcome, this stuff is very 
> brittle! I'm almost at a
> point having the tree duplicated for every boost version, for not having to 
> recompile kicad
> completely when switching them...
> 
> Michael

Hi Michael,
I just test your new fix on 3 installs :
W7 32 bits & boost 1.60, Linux Kubuntu 14.04LTS & boost 1.54,  Linux Ubuntu 
16.04 & boost 1.58.

All work fine after applying the new patch. (All crashed before the patch)
(I do not have an installed version of boost 1.61)

Really thanks. This is the type of issue very tricky to fix.

I also tested your refactor without issue.
Just I had a compil warning on Linux (attached the small file)


-- 
Jean-Pierre CHARRAS
In file included from 
/home/jpc/kicad-launchpad/testing/common/tool/tool_manager.cpp:45:0:
/home/jpc/kicad-launchpad/testing/include/tool/coroutine.h: In instantiation of 
'COROUTINE::COROUTINE(std::function) 
[with ReturnType = int; ArgType = const TOOL_EVENT&]':
/home/jpc/kicad-launchpad/testing/common/tool/tool_manager.cpp:541:96:   
required from here
/home/jpc/kicad-launchpad/testing/include/tool/coroutine.h:332:19: warning: 
'COROUTINE::m_callee' will be initialized after 
[-Wreorder]
 context_type* m_callee;
   ^
/home/jpc/kicad-launchpad/testing/include/tool/coroutine.h:325:16: warning:   
'int COROUTINE::m_retVal' [-Wreorder]
 ReturnType m_retVal;
^
/home/jpc/kicad-launchpad/testing/include/tool/coroutine.h:123:5: warning:   
when initialized here [-Wreorder]
 COROUTINE( std::function aEntry ) :
 ^
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] About Bug 1604841: Pcbnew crash at moving via, and boost::context fixes to make it compatible with boost 1.61

[jp charras]

Le 01/08/2016 à 05:15, Michael Steinberg a écrit :
> Hey,
> 
> pinpointing the problem proved really difficult, I didn't expect it where it 
> was...
> This bug existed before my 1.61-patch and did not surface because DELEGATE 
> would not clear its
> content on destruction. See the attached patch.
> 
> That being said, I'm not yet 100% sure the macro'ed code for old boost 
> versions is 100% correct.
> During my bug search I refactored and checked that part for [1.54, 1.66) 
> since I suspected my stuff
> being the cause. I'll check with 1.56 later, need to catch some sleep first...
> 
> Michael

Thanks.
I tried your patch (W7 32 bits boots 1.60).
Unfortunately, there is still a crash:
The first time I drag a via or track, it works.
But the second time, I have a crash with the message on the console:

terminate called after throwing an instance of 'std::bad_function_call'
  what():  bad_function_call

Sorry,

-- 
Jean-Pierre CHARRAS

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] About Bug 1604841: Pcbnew crash at moving via, and boost::context fixes to make it compatible with boost 1.61

[Michael Steinberg]

Hi there!

Thank you for investigating the problem. After reading your explanation,
I am not completely sure the bug has existed before the boost-1.61
patch. In the mentioned patch, the order of transitions.clear() and
COROUTINE constructor has been reversed [1]. Do you think it could cause
the bug? If so, then the stable branch never had this bug and as such
should be safe.

I am still unable to recreate the problem, so could you simply try to
revert the order again to see if it solves the issue?

It's the "st->Push()" operation that eventually calls 
"st->transitions.clear()". When writing the 1.61 patch I must have run 
into 50% of the problem and changed the order of the *explicit* 
st->transistions.clear(), not realizing that Push does it as well. So 
before my patch it might have been cleared twice before dereferencing a 
referenced element. I don't suspect this to have caused any issues, 
since clear usually only calls the destructors and keeps the memory 
reserved, which then would silently work with the implementation of 
DELEGATE.


Michael

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] About Bug 1604841: Pcbnew crash at moving via, and boost::context fixes to make it compatible with boost 1.61

[Maciej Sumiński]

Hi Wayne, Jean-Pierre,

Excuse my late answer, I was away for a week.

I could not reproduce the problem here, so right now I am unable to say
whether it fixes the recent issue. I will try again later with another
virtual machine. I believe the stable branch is not affected by the bug,
but we will get the definite answer a bit later (hopefully today).

Regards,
Orson

On 08/01/2016 02:31 PM, Wayne Stambaugh wrote:
> Tom or Orson,
> 
> Would you please take a look at this patch and see if makes sense?  I'm
> wondering if resolves any of the other GAL bugs we have been experiencing.
> 
> Thanks,
> 
> Wayne
> 
> On 7/31/2016 11:15 PM, Michael Steinberg wrote:
>> Hey,
>>
>> pinpointing the problem proved really difficult, I didn't expect it
>> where it was...
>> This bug existed before my 1.61-patch and did not surface because
>> DELEGATE would not clear its content on destruction. See the attached
>> patch.
>>
>> That being said, I'm not yet 100% sure the macro'ed code for old boost
>> versions is 100% correct. During my bug search I refactored and checked
>> that part for [1.54, 1.66) since I suspected my stuff being the cause.
>> I'll check with 1.56 later, need to catch some sleep first...
>>
>> Michael




signature.asc
Description: OpenPGP digital signature
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] About Bug 1604841: Pcbnew crash at moving via, and boost::context fixes to make it compatible with boost 1.61

[Maciej Sumiński]

On 08/01/2016 05:15 AM, Michael Steinberg wrote:
> Hey,
> 
> pinpointing the problem proved really difficult, I didn't expect it
> where it was...
> This bug existed before my 1.61-patch and did not surface because
> DELEGATE would not clear its content on destruction. See the attached
> patch.
> 
> That being said, I'm not yet 100% sure the macro'ed code for old boost
> versions is 100% correct. During my bug search I refactored and checked
> that part for [1.54, 1.66) since I suspected my stuff being the cause.
> I'll check with 1.56 later, need to catch some sleep first...
> 
> Michael

Michael,

Thank you for investigating the problem. After reading your explanation,
I am not completely sure the bug has existed before the boost-1.61
patch. In the mentioned patch, the order of transitions.clear() and
COROUTINE constructor has been reversed [1]. Do you think it could cause
the bug? If so, then the stable branch never had this bug and as such
should be safe.

I am still unable to recreate the problem, so could you simply try to
revert the order again to see if it solves the issue?

Regards,
Orson

1.
https://github.com/KiCad/kicad-source-mirror/commit/06d4894fdbeb00727cdcc667b8899ad73d8eb1c2#diff-44c36eb97bef35a82242eba1b55fb1d4R539



signature.asc
Description: OpenPGP digital signature
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] About Bug 1604841: Pcbnew crash at moving via, and boost::context fixes to make it compatible with boost 1.61

[Wayne Stambaugh]

Tom or Orson,

Would you please take a look at this patch and see if makes sense?  I'm
wondering if resolves any of the other GAL bugs we have been experiencing.

Thanks,

Wayne

On 7/31/2016 11:15 PM, Michael Steinberg wrote:
> Hey,
> 
> pinpointing the problem proved really difficult, I didn't expect it
> where it was...
> This bug existed before my 1.61-patch and did not surface because
> DELEGATE would not clear its content on destruction. See the attached
> patch.
> 
> That being said, I'm not yet 100% sure the macro'ed code for old boost
> versions is 100% correct. During my bug search I refactored and checked
> that part for [1.54, 1.66) since I suspected my stuff being the cause.
> I'll check with 1.56 later, need to catch some sleep first...
> 
> Michael
> 
> 
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> 

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] About Bug 1604841: Pcbnew crash at moving via, and boost::context fixes to make it compatible with boost 1.61

[Michael Steinberg]

Hey,

pinpointing the problem proved really difficult, I didn't expect it 
where it was...
This bug existed before my 1.61-patch and did not surface because 
DELEGATE would not clear its content on destruction. See the attached patch.


That being said, I'm not yet 100% sure the macro'ed code for old boost 
versions is 100% correct. During my bug search I refactored and checked 
that part for [1.54, 1.66) since I suspected my stuff being the cause. 
I'll check with 1.56 later, need to catch some sleep first...


Michael
>From 8d0b28ead9133c3e808f06d914463449fd6f08e0 Mon Sep 17 00:00:00 2001
From: decimad 
Date: Mon, 1 Aug 2016 05:11:29 +0200
Subject: [PATCH] Fix tool crash with "reentrant" states.

---
 common/tool/tool_manager.cpp | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/common/tool/tool_manager.cpp b/common/tool/tool_manager.cpp
index 0ec1882..46594da 100644
--- a/common/tool/tool_manager.cpp
+++ b/common/tool/tool_manager.cpp
@@ -532,11 +532,18 @@ void TOOL_MANAGER::dispatchInternal( const TOOL_EVENT& aEvent )
 {
 if( tr.first.Matches( aEvent ) )
 {
+// st->Push() might clear the transitions-list, copy the transition function
+// early. Can move the function since the transition list will be cleared.
+using coroutine_type = COROUTINE< int, const TOOL_EVENT& >;
+std::unique_ptr< coroutine_type > next_cofunc( 
+new coroutine_type( std::move( tr.second ) )
+);
+
 // if there is already a context, then store it
 if( st->cofunc )
 st->Push();
 
-st->cofunc = new COROUTINE( tr.second );
+st->cofunc = next_cofunc.release();
 
 // as the state changes, the transition table has to be set up again
 st->transitions.clear();
-- 
2.9.0.windows.1

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] About Bug 1604841: Pcbnew crash at moving via, and boost::context fixes to make it compatible with boost 1.61

[Michael Steinberg]

Hello,

of course! So I'm not to sure if 1.60 is supposed to work on windows, I 
think I read it might be bugged for that specific version and platform 
combination. I will investigate that. However the intent was to not 
change anything behaviorial for boost version < 1.61, so if a 
1.54-version build is not working after that patch, then the macro'ed 
code is wrong. I apologize sincerely and investigate subtleties tonight.


Michael


Am 31.07.2016 um 17:14 schrieb jp charras:

Orson, Michael:

May I ask you to have a look into this bug:
In gal mode, Pcbnew crashes when try to drag a via or a track.

To reproduce that, set the interactive Router Setting/ Mouse drag behavior to 
interactive drag.
(In move mode, no crash)

I am asking you to have a look into this issue because the version rev 6964 
does not crash.
The rev 6965 is the  boost::context fixes to make it compatible with boost 1.61.

I did not tested the rev 6965 but I am afraid this bug is related to this fix.

The crash occurs both on my Windows install (boost 1.60) and on my Linux 
install (boost 1.54)

Unfortunately the back trace is not useful, but it shows the crash is related 
to changes in rev 6965.

If it is really due to the rev 6965, this is a serious issue.

Thanks.




___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp