dynamorio
dynamorio copied to clipboard
DynamoRIO
i#4399: Adds Debug Assertion
Adds a DR_DEBUG_ASSERT which does not assert when DR_NDEBUG is defined.
DR_NDEBUG is a new definition that enables users to turn on/off debug functionality exposed by DR's interface.
Fixes: #4399
I'll modify the extensions to use this assertion once we agree on its format.
Not sure if I am doing something wrong or if a compiler bug is present, but if I use the #ifdef #else, I get a compilation error. Separating the checks seems to work.
@derekbruening Any idea what is going on here please?
Not sure if I am doing something wrong or if a compiler bug is present, but if I use the #ifdef #else, I get a compilation error. Separating the checks seems to work.
@derekbruening Any idea what is going on here please?
Both genapi.pl and doxygen go through headers and interpret ifdefs: both have lists of what to consider defined; genapi has lists of what to take both sides of. Probably it's one of those; probably genapi.
@derekbruening Not sure if you want to take a preliminary look before I start changing the code of the extensions please. No rush.
Should we deprecate DR_ASSERT, add an alias DR_CHECK or DR_RELEASE_ASSERT, recommend new code use that, and change all our own existing code to use that? So then we'd have DR_DEBUG_ASSERT and DR_RELEASE_ASSERT (or DR_CHECK or sthg) and there's no confusion over "ASSERT" being in release build.
DR_ASSERT
Do you mean that DR_ASSERT, as a name, is potentially confusing to users and therefore is the reason why you want to deprecate it?
DR_ASSERT
Do you mean that DR_ASSERT, as a name, is potentially confusing to users and that is the reason why you want to deprecate it?
Right: I think generally people expect an "assert" to be debug-only. Or is that not always the case?
Thinking about it now, one way is to adjust slightly my PR to adopt the <assert.h> model, i.e., keep DR_ASSERT (and remove my DR_DEBUG_ASSERT) and toggle whether it asserts or does nothing via -DDR_NDEBUG
Thinking about it now, one way is to adjust slightly my PR to adopt the <assert.h> model, i.e., keep DR_ASSERT (and remove my DR_DEBUG_ASSERT) and toggle whether it asserts or does nothing via -DDR_NDEBUG
Ofc, the downside is that we break backwards compatibility.
Thinking about it now, one way is to adjust slightly my PR to adopt the <assert.h> model, i.e., keep DR_ASSERT (and remove my DR_DEBUG_ASSERT) and toggle whether it asserts or does nothing via -DDR_NDEBUG
Hmm, I originally assumed we'd have to leave the existing behavior alone, for compatibility: tools might rely on release-build checks at this point. I think you would have to not turn on -DDR_NDEBUG for DEBUG=off in cmake then, right? Or we could have a version check or a DR variable (like DynamoRIO_REG_COMPATIBILITY) to have it on for DEBUG=off for all recompiled code (but then someone just rebuilding would hit it).
How about we treat DR_NDEBUG as a cflag option that is only set via the command-line. If the user does not set it when invoking cmake .., backwards compatibility of DR_ASSERT is maintained.
How about we treat DR_NDEBUG as a cflag option that is only set via the command-line. If the user does not set it when invoking cmake .., backwards compatibility of DR_ASSERT is maintained.
OK, and we audit all the uses in our own code base and replace ones we want in release with the release version, and we add DR_NDEBUG to package.cmake and runsuite.cmake, and document it of course?
replace ones we want in release with the release version,
Oh, so DR does rely on DR_ASSERT for release builds? That's right, I think I can recall some places, particularly in init functions associated with exts.
How about we treat DR_NDEBUG as a cflag option that is only set via the command-line. If the user does not set it when invoking cmake .., backwards compatibility of DR_ASSERT is maintained.
OK, and we audit all the uses in our own code base and replace ones we want in release with the release version, and we add DR_NDEBUG to package.cmake and runsuite.cmake, and document it of course?
So just to confirm that we are on the same page please; we are okay with breaking the backwards compatibility of DR_ASSERT on new releases right? or do you think there is a neater way of doing this?
How about we treat DR_NDEBUG as a cflag option that is only set via the command-line. If the user does not set it when invoking cmake .., backwards compatibility of DR_ASSERT is maintained.
OK, and we audit all the uses in our own code base and replace ones we want in release with the release version, and we add DR_NDEBUG to package.cmake and runsuite.cmake, and document it of course?
So just to confirm that we are on the same page please; we are okay with breaking the backwards compatibility of DR_ASSERT on new releases right? or do you think there is a neater way of doing this?
By "new releases" you just mean our own uses of DR_ASSERT in the binaries we include in new release packages, and not changing behavior of other users who take our new release and recompile their own code?
How about we treat DR_NDEBUG as a cflag option that is only set via the command-line. If the user does not set it when invoking cmake .., backwards compatibility of DR_ASSERT is maintained.
OK, and we audit all the uses in our own code base and replace ones we want in release with the release version, and we add DR_NDEBUG to package.cmake and runsuite.cmake, and document it of course?
So just to confirm that we are on the same page please; we are okay with breaking the backwards compatibility of DR_ASSERT on new releases right? or do you think there is a neater way of doing this?
By "new releases" you just mean our own uses of DR_ASSERT in the binaries we include in new release packages, and not changing behavior of other users who take our new release and recompile their own code?
Yes. Like you said before, we'll need to audit our use of DR_ASSERT.
How about we treat DR_NDEBUG as a cflag option that is only set via the command-line. If the user does not set it when invoking cmake .., backwards compatibility of DR_ASSERT is maintained.
OK, and we audit all the uses in our own code base and replace ones we want in release with the release version, and we add DR_NDEBUG to package.cmake and runsuite.cmake, and document it of course?
So just to confirm that we are on the same page please; we are okay with breaking the backwards compatibility of DR_ASSERT on new releases right? or do you think there is a neater way of doing this?
By "new releases" you just mean our own uses of DR_ASSERT in the binaries we include in new release packages, and not changing behavior of other users who take our new release and recompile their own code?
Yes. Like you said before, we'll need to audit our use of DR_ASSERT.
Yes, changing how some errors are reported in some of our own ext/ libraries seems fine.
@johnfxgalea just going through the still-open PR's: this still seems like a useful change as I keep being surprised at these assert macros being enabled in release build; did you have plans to revive this?