Opened 5 years ago

Closed 17 months ago

#11184 closed enhancement (fixed)

Trap directly in the function where wxASSERT is

Reported by: faustus Owned by:
Priority: low Milestone:
Component: base Version: stable-latest
Keywords: assert debug trap Cc:
Blocked By: Blocking:
Patch: yes

Description

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.

Attachments (2)

faustus_wxAssert.patch download (19.6 KB) - added by faustus 5 years ago.
assert_inplace.diff download (21.0 KB) - added by vadz 4 years ago.
Updated version of the patch for r64571

Download all attachments as: .zip

Change History (14)

comment:1 Changed 5 years ago by vadz

  • Component changed from wxMSW to base
  • Keywords assert debug trap added
  • Status changed from new to confirmed

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!

comment:2 Changed 5 years ago by 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 :)

comment:3 Changed 5 years ago by faustus

please take last patch.

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

comment:4 Changed 5 years ago by 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.

comment:5 Changed 5 years ago by vadz

  • Status changed from confirmed to infoneeded_new

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?

comment:6 Changed 5 years ago by faustus

  • Status changed from infoneeded_new to new

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

comment:7 Changed 5 years ago by faustus

done, please take the updated patch.

comment:8 Changed 5 years ago by vadz

  • Milestone set to 2.9.1

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.

Changed 5 years ago by faustus

comment:9 Changed 5 years ago by faustus

restored xSetDefaultAssertHandler()... sorry for that

comment:10 Changed 4 years ago by vadz

  • Milestone 2.9.1 deleted
  • Priority changed from normal to low
  • Status changed from new to confirmed
  • Summary changed from wxASSERT_MSG inplace to Trap directly in the function where wxASSERT is

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).

Changed 4 years ago by vadz

Updated version of the patch for r64571

comment:11 Changed 17 months ago by 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.

comment:12 Changed 17 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(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.

Note: See TracTickets for help on using tickets.