|
From: Uttam P. <ut...@us...> - 2006-01-20 00:14:46
|
Hi All,
I've a patch ready to submit which has few cleanup changes. If you look at the patch, the code in
if (verboze) condition is a dead code as verboze is initialized 'False'. I can understand that, the
code in the condition is some kind of debug output which might be necessary from time to time to
print the intermediate debug output. So my proposal is to make this output available with -D compiler
option. So if somebody wants to enable the extra output he/she can make debug build (which has some
extra option with -D) instead of changing the code everytime he/she wants to enable/disable this
output. Going further we can use this flag in other places in the code to plant some intermediate
checkpoints,
I would appreciate any thoughts/comments.
thanks,
Uttam
--- valgrind-3.1.0.orig/memcheck/mc_translate.c 2006-01-20 07:58:06.888061336 -0800
+++ valgrind-3.1.0/memcheck/mc_translate.c 2006-01-20 08:00:17.811128976 -0800
@@ -2937,7 +2937,6 @@ IRBB* MC_(instrument) ( IRBB* bb_in, Vex
Addr64 orig_addr_noredir, VexGuestExtents* vge,
IRType gWordTy, IRType hWordTy )
{
- Bool verboze = False; //True;
Bool bogus;
Int i, j, first_stmt;
IRStmt* st;
@@ -3008,11 +3007,6 @@ IRBB* MC_(instrument) ( IRBB* bb_in, Vex
st = bb_in->stmts[i];
first_stmt = bb->stmts_used;
- if (verboze) {
- ppIRStmt(st);
- VG_(printf)("\n\n");
- }
-
/* Generate instrumentation code for each stmt ... */
switch (st->tag) {
@@ -3069,15 +3063,6 @@ IRBB* MC_(instrument) ( IRBB* bb_in, Vex
} /* switch (st->tag) */
- if (verboze) {
- for (j = first_stmt; j < bb->stmts_used; j++) {
- VG_(printf)(" ");
- ppIRStmt(bb->stmts[j]);
- VG_(printf)("\n");
- }
- VG_(printf)("\n");
- }
-
/* ... and finally copy the stmt itself to the output. */
addStmtToIRBB(bb, st);
@@ -3086,23 +3071,8 @@ IRBB* MC_(instrument) ( IRBB* bb_in, Vex
/* Now we need to complain if the jump target is undefined. */
first_stmt = bb->stmts_used;
- if (verboze) {
- VG_(printf)("bb->next = ");
- ppIRExpr(bb->next);
- VG_(printf)("\n\n");
- }
-
complainIfUndefined( &mce, bb->next );
- if (verboze) {
- for (j = first_stmt; j < bb->stmts_used; j++) {
- VG_(printf)(" ");
- ppIRStmt(bb->stmts[j]);
- VG_(printf)("\n");
- }
- VG_(printf)("\n");
- }
-
return bb;
}
|
|
From: Tom H. <to...@co...> - 2006-01-20 00:20:46
|
In message <113...@li...te>
Uttam Pawar <ut...@us...> wrote:
> I've a patch ready to submit which has few cleanup changes. If you look at
> the patch, the code in if (verboze) condition is a dead code as verboze is
> initialized 'False'. I can understand that, the code in the condition is
> some kind of debug output which might be necessary from time to time to
> print the intermediate debug output. So my proposal is to make this output
> available with -D compiler option. So if somebody wants to enable the extra
> output he/she can make debug build (which has some extra option with -D)
> instead of changing the code everytime he/she wants to enable/disable this
> output. Going further we can use this flag in other places in the code to
> plant some intermediate checkpoints,
This (having a static variable initialised to false which guards
debug code) is a common technique in valgrind and seems perfectly
reasonable to me. Whay do you want to change to a preprocessor
conditional?
One advantage of the way we do it at the moment is that the debug
code can't get out of date as it is always parsed by the compiler
even when debugging is not on.
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
|
From: Robert W. <rj...@du...> - 2006-01-20 00:43:15
|
On Thu, 2006-01-19 at 16:17 -0800, Uttam Pawar wrote:
> Hi All,
>
> I've a patch ready to submit which has few cleanup changes. If you look at the patch, the code in
> if (verboze) condition is a dead code as verboze is initialized 'False'. I can understand that, the
> code in the condition is some kind of debug output which might be necessary from time to time to
> print the intermediate debug output. So my proposal is to make this output available with -D compiler
> option. So if somebody wants to enable the extra output he/she can make debug build (which has some
> extra option with -D) instead of changing the code everytime he/she wants to enable/disable this
> output. Going further we can use this flag in other places in the code to plant some intermediate
> checkpoints,
>
> I would appreciate any thoughts/comments.
This behavior is intentional. Code like this:
if(verbose)
do_verbose_stuff();
will be syntax-checked/type-checked/etc. by the compiler. It won't be
an optimization problem, as the compiler will notice that the code is
dead and completely optimized out.
However, this code:
#ifdef VERBOSE
do_verbose_stuff();
#endif
will not be syntax-checked/type-checked/etc. by the compiler unless
VERBOSE is true. This means verbose code will bitrot when changes
happen (variables go away, change names, etc.) as most people won't
remember or care about compiling with VERBOSE defined.
Regards,
Robert.
|
|
From: Uttam P. <ut...@us...> - 2006-01-20 01:08:58
|
> This behavior is intentional. Code like this: > if(verbose) > do_verbose_stuff(); > > will be syntax-checked/type-checked/etc. by the compiler. It won't be > an optimization problem, as the compiler will notice that the code is > dead and completely optimized out. Agree. > However, this code: > > #ifdef VERBOSE > do_verbose_stuff(); > #endif > > will not be syntax-checked/type-checked/etc. by the compiler unless > VERBOSE is true. This means verbose code will bitrot when changes > happen (variables go away, change names, etc.) as most people won't > remember or care about compiling with VERBOSE defined. > True. Thanks for the feedback. Uttam |
|
From: Julian S. <js...@ac...> - 2006-01-20 02:18:06
|
Uttam > On Friday 20 January 2006 00:43, Robert Walsh wrote: > > This behavior is intentional. Code like this: > > if(verbose) > do_verbose_stuff(); > > will be syntax-checked/type-checked/etc. by the compiler. It won't be > an optimization problem, as the compiler will notice that the code is > dead and completely optimized out. > > However, this code: > > #ifdef VERBOSE > do_verbose_stuff(); > #endif > > will not be syntax-checked/type-checked/etc. by the compiler unless > VERBOSE is true. This means verbose code will bitrot when changes > happen (variables go away, change names, etc.) as most people won't > remember or care about compiling with VERBOSE defined. Absolutely my thoughts too. I can only add that I as a corporate employee in a previous life, I was involved in a project which nearly collapsed into a total screaming nightmare of #ifdefs -- the code was essentially unmaintainable -- and I have been keen to avoid them ever since. If you think this is a dead code problem, just consider that in VEX/ all the front ends and back ends are always compiled in -- tens of thousands of useless lines of code -- even though only one front end and one back end will be used by any particular valgrind executable. J |