Trap directly in the function where wxASSERT is

[faustus]

I changed the behavior of wxASSERT_MSG
in order to have the debugger exaclty on the caller macro and not inside
the calling stack of assert functions and dialogs.
Mainly i made all return type of "wxOnAssert" from void to int
to allow the debugbreak macro to be inplace.
I only tested this on msw and also i'm not sure if the place where i put the macro is right.

[vadz]

Thanks for the patch, it indeed would be more convenient if the debugger opened directly on the assert macro. Unfortunately I'm not sure if __asm int 3 is really portable: do all Windows compilers support __asm like this? And what about x86_64, does int 3 still generate debug breakpoint there? The proper way would be to use DebugBreak() just as wxTrap() does but this would require including windows.h which we want to avoid. The same applies to the Mac solution, Debugger() function also requires including system headers so right now the code probably won't compile at all under Mac.

I think a reasonable compromise could be to call wxTrap() from assert macros themselves. This would still mean you need to go up one level in the debugger but just one and not several as now -- IMO this is reasonable. What do you think?

Also, I have currently a lot of pending changes to this part of the code (see  this thread) so if you're going to redo the patch as suggested above it's probably worth waiting until I check them in (this week-end, I think).

Thanks!

[faustus]

On win32 we can use intrinsic function __debugbreak defined in WinNT.h i tried and it works

on Mac i don't know if there is an equivalent intrinsic or macro. In that case we can map debugbreak macro

with wxTrap(). Please check my updated patch.

I will check the thread too. I work with wxwidgets since 2004 but now i chose to dedicate some of my time improving and fixing bug. It's really a great projects and i love them. We use actually in our internal 3d Game Engine and Editor... we develop videogames :)

[faustus]

please take last patch.

now assert dialog return wxYES,wxNO,wxCANCEL to better manage static s_bNoAsserts

[vadz]

I didn't know about __debugbreak(), it looks like this is indeed the best solution for MSVC. Unfortunately it (probably, I didn't test it) won't work for any other compiler so we'd still need to use wxTrap() for them.

[vadz]

As mentioned in my first reply, this patch doesn't apply any longer after the merge of my debug build changes so could you please update it against the current svn trunk? Also, as I wrote in comment:4, __debugbreak() is unlikely to be supported by all Windows compilers so we should only use it if __VISUALC__ is defined. I suggest defining wxTrap() as a macro if it can be done inline (e.g. in MSVC case) but defining it as a function otherwise. In any case, the assert macros should use wxTrap() and not debug_break which is redundant (it does the same thing) and can result in name clashes (as it doesn't have a wx prefix).

Finally, there are some minor style issues in the patch: could you please use var = value; instead of var=value; and wxFalse instead of 0 in the dummy while statements.

Thanks in advance!

P.S. Are you the same faustus as  this one? I.e. should I use "David Lange" name when crediting your changes?

[faustus]

hi vadz, ok i'm going to update against the current trunk and take all your suggestion.

You can find some information about me here:  http://www.linkedin.com/in/faustus

Ciao,

Fausto

[faustus]

done, please take the updated patch.

[vadz]

I just realized that this patch introduces a compatibility problem by changing the return type of virtual OnAssertFailure() -- so any code overriding it will need to be changed. However at least this breakage is not silent so maybe we can live with it? Overriding this function doesn't seem to be very popular according to  my search but at least wxPython would need to be changed.

It does make slightly more sense to return whether we should trap or not from the handler instead of trapping inside it itself and I don't see any other way to achieve what this patch does while still preserving the void return type. Well, actually I can: by using a global variable. But this is really ugly...

What do the others think?

P.S. Note to self: the patch was apparently done after an incorrect merge and removes the recently added wxSetDefaultAssertHandler(), do not apply this part of it.

[faustus]

restored xSetDefaultAssertHandler()... sorry for that

[vadz]

I've updated the patch to apply to the latest trunk (see attachment) but I didn't apply it because it still needs to be documented (and it would be also nice to improve inline comments). I am also worried about breaking the user code which overrides OnAssert[Failure]() as it would be broken (albeit not silently) by this change. I wonder if there is some way to avoid this? Finally, while this patch improves the behaviour when you break into the debugger, we still show various assert-related functions in the message box shown by wx itself. It seems like we ought to avoid this too, as now these functions are not even seen in the stack trace under debugger at all.

If anybody wants to improve this patch further, please do it. But in the meanwhile this is not critical for 2.9.1 nor even 3.0 (although it would be nice to have).

[vadz]

Updated version of the patch for r64571

[vadz]

OK, finally it seems that quite a few projects override OnAssertFailure() so it's probably not a good idea to change its signature. I'm going to use an ugly but compatible workaround with a global flag to pass the information about whether we should trap from the default handler to the macro instead.

[VZ]

(In [73124]) Open debugger at the location of failing assert, if possible.

Break into the debugger in the function containing the assert that failed
instead of inside wxWidgets assert handler which is several (~8) levels below
the last line of the user code. This is much more useful in practice and also
less confusing.

Currently this only works for MSVC as the other compilers don't have any
debugbreak intrinsice equivalent.

Also update the except sample to test wxTrap() directly too.

Closes #11184.