[Maria-developers] RFC: Compile-time checking for unmatched DBUG_ENTER/LEAVE via unused variable
![](https://secure.gravatar.com/avatar/e66085296f0242482a6e3b441d551d7e.jpg?s=120&d=mm&r=g)
Maria developers/captains, Prior to posing it as a final patch contribution or opening a JIRA ticket, I wanted to propose this idea. The attached patch adds what amounts to compile-time checking for unmatched DBUG_ENTER/DBUG_LEAVE (DBUG_RETURN, DBUG_VOID_RETURN) by introducing a variable in DBUG_ENTER which is only used in DBUG_LEAVE. This allows any compiler which can robustly detect unused variables to detect the mismatch at compile-time. There is already a run-time check for this case, but it is somewhat limited as it requires _db_return_ to be called in order to detect the mismatch, and this is in practice not always the case. Particularly three cases allow this to escape detection: 1. Some instrumented functions are called only from non-instrumented functions. 2. Some instrumented functions are called only from non-returning functions, such as abort or die functions. 3. Some instrumented functions are called from functions who call my_end or my_thread_end (making cs unavailable) before returning. In any case please see the attached DRAFT patch for a full implementation of the idea. Note one bit of ugliness is that non-returning functions, after this patch, must be marked explicitly as DBUG_ENTER_NO_RETURN -- this actually is not really a bad thing, though in and of itself. Regards, Jeremy
![](https://secure.gravatar.com/avatar/548f6a7ca82fc307ee146f412b04905c.jpg?s=120&d=mm&r=g)
20.07.2013 00:48, Jeremy Cole пишет:
Maria developers/captains,
Prior to posing it as a final patch contribution or opening a JIRA ticket, I wanted to propose this idea.
The attached patch adds what amounts to compile-time checking for unmatched DBUG_ENTER/DBUG_LEAVE (DBUG_RETURN, DBUG_VOID_RETURN) by introducing a variable in DBUG_ENTER which is only used in DBUG_LEAVE. This allows any compiler which can robustly detect unused variables to detect the mismatch at compile-time. There is already a run-time check for this case, but it is somewhat limited as it requires _db_return_ to be called in order to detect the mismatch, and this is in practice not always the case. Particularly three cases allow this to escape detection:
I like the idea. People who are responsible for DBUG_* are on vacation I hope they will like the patch too.
![](https://secure.gravatar.com/avatar/2c85cb4200b53244a77d3f7854818abe.jpg?s=120&d=mm&r=g)
Jeremy Cole
The attached patch adds what amounts to compile-time checking for unmatched DBUG_ENTER/DBUG_LEAVE (DBUG_RETURN, DBUG_VOID_RETURN) by introducing a variable in DBUG_ENTER which is only used in DBUG_LEAVE. This allows any compiler which can robustly detect unused variables to detect the mismatch at compile-time. There is already a run-time check for this case, but it is somewhat limited as it requires _db_return_ to be called in order to detect the mismatch, and this is in practice not always the case. Particularly three cases allow this to escape detection:
Ideally we'd be able to check for gcc attribute noreturn but I'm not sure there's a way to do that (as it's not a preproceessor thing). I'm pretty sure that with some optimization levels that the temp variable can get optimized out (although I haven't looked at the produced code). Personally, looks good and I'm tempted to pull it into Percona Server. -- Stewart Smith
![](https://secure.gravatar.com/avatar/e66085296f0242482a6e3b441d551d7e.jpg?s=120&d=mm&r=g)
Stewart,
You're right that it's entirely up to the optimizer to optimize out that
variable (and in fact I hope that it would). That should not change the
"unused-variable" warning though, as that static analysis done by the
compiler happens much earlier, as far as I know. That is, the detection of
the variable being unused should be completely independent of the
optimizer's decision to eliminate the variable entirely.
Regards,
Jeremy
On Wed, Aug 7, 2013 at 2:14 AM, Stewart Smith
Jeremy Cole
writes: The attached patch adds what amounts to compile-time checking for unmatched DBUG_ENTER/DBUG_LEAVE (DBUG_RETURN, DBUG_VOID_RETURN) by introducing a variable in DBUG_ENTER which is only used in DBUG_LEAVE. This allows any compiler which can robustly detect unused variables to detect the mismatch at compile-time. There is already a run-time check for this case, but it is somewhat limited as it requires _db_return_ to be called in order to detect the mismatch, and this is in practice not always the case. Particularly three cases allow this to escape detection:
Ideally we'd be able to check for gcc attribute noreturn but I'm not sure there's a way to do that (as it's not a preproceessor thing).
I'm pretty sure that with some optimization levels that the temp variable can get optimized out (although I haven't looked at the produced code).
Personally, looks good and I'm tempted to pull it into Percona Server. -- Stewart Smith
![](https://secure.gravatar.com/avatar/39b623a1559cf9c69ac3d9d4fb44e7fe.jpg?s=120&d=mm&r=g)
Hi, Jeremy! On Jul 19, Jeremy Cole wrote:
Prior to posing it as a final patch contribution or opening a JIRA ticket, I wanted to propose this idea.
The attached patch adds what amounts to compile-time checking for unmatched DBUG_ENTER/DBUG_LEAVE (DBUG_RETURN, DBUG_VOID_RETURN) by introducing a variable in DBUG_ENTER which is only used in DBUG_LEAVE. This allows any compiler which can robustly detect unused variables to detect the mismatch at compile-time. There is already a run-time check for this case, but it is somewhat limited as it requires _db_return_ to be called in order to detect the mismatch, and this is in practice not always the case. Particularly three cases allow this to escape detection:
1. Some instrumented functions are called only from non-instrumented functions. 2. Some instrumented functions are called only from non-returning functions, such as abort or die functions. 3. Some instrumented functions are called from functions who call my_end or my_thread_end (making cs unavailable) before returning.
In any case please see the attached DRAFT patch for a full implementation of the idea. Note one bit of ugliness is that non-returning functions, after this patch, must be marked explicitly as DBUG_ENTER_NO_RETURN -- this actually is not really a bad thing, though in and of itself.
Thanks, Jeremy! I kind of liked the idea. Although I, probably, wouldn't use DBUG_ENTER_NO_RETURN, but a separate macro DBUG_NO_RETURN that would use your guard variable without calling _db_return_. Hmm, come to think of it, perhaps functions that don't return should still use DBUG_LEAVE? Then no special new macro is needed at all. But what made me thinking was, that this patch only detects whether the function has at least one DBUG_RETURN, anywhere in the function body (your changeset comment mentions this, albeit in a wrong place, so it's a bit confusing). In my experience a notably more popular mistake is to miss DBUG_RETURN only somewhere in a function, like in the early return on the error. What could we do to solve this too? Is C++ class the best we could have? Regards, Sergei
participants (4)
-
Jeremy Cole
-
Oleksandr Byelkin
-
Sergei Golubchik
-
Stewart Smith