tags:

views:

297

answers:

10

Suppose there is a function with 1000 code of line named LongFunction, and we used it:

bool bSuccess = LongFunction();
assert(bSuccess);

Here I got an assert when debugging, and I know there is something wrong with LongFunction, so I need to find where the function meet problems and returns:

  1. I could probably debug it step by step, it works but is time-consuming, we don't what to do it this way.

  2. I could search the keyword "return" (or even more refined search use RegExp), and set breakpoint at those returns, it should be faster, but it is still tedious manual work which can't be automated.

  3. #define return TRACE(LINE); return

It works but have following problems:

  • It will print too much redundant information as return is frequently used.(Or we could use some EnvVar to switch it on or off)
  • Does't work for following case: if(bOK) return true;

Do you have any other creative ideas on how to pinpoint the problem?

Edit: Here are some details to let us focus on the problem.

  1. It is about C++, and not platform specfic.

  2. We don't want to refactoring the functions(Yea, I know we should), we even don't want to change any code - at this point we just want to provide some facility to make our application debugging easier. I also believe this should be a common requirement, don't you ever run into this?

  3. The LongFunction() have multiple exit point, and the return type is not necessay bool(HRESULT, user defined errorcode...)

Edit: A summary of current discussions:
We have some controversies:

  1. You should refactor the function.
    Yea, everyone know that we should, but that is not the point.If I had make the call to refactor the function, I won't be here to ask the question.

  2. Find where the LongFunction() returns failure doesn't help.
    It is always the first thing I do to locate where the error occurs to know what happened, I am curious why this doesn't help, what did you do in this situation? (Assume I am already familiar with how the function works)

And we have 2 reasonable solutions:

  1. ReturnMarker from Crashworks, a stack object in the function will destruct when function returns, set the breakpoint at the destructor will show you where it returns in debuger

  2. CMyBool(x) from Binary & Sadsido, change the return type of LongFunction to CMyBool which can contruct from a bool, return from LongFunction will contruct that object, so just set a breakpoint at the constructor will work.

+8  A: 

Sounds like it's time to refactor LongFunction()...

A 1000 line function is a bad code smell. Spend the time refactoring it into smaller, more maintainable functions. You'll find the bug(s) while you're at it, and it will be a worthwhile investment for the future.

Roddy
A very good answer for the question "Should I refactor my 1000-line function?". But the question is "How to pinpoint where a 1000-line function returns?"...
SadSido
@sadsido: That was the question, but it's not actually the *problem*... The problem is a chunk of code that's not behaving as expected, and seems to be not well structured/documented/instrumented/etc.
Roddy
There's an old Irish joke. Guy is lost out deep farmland in the West of Ireland, he and stops at a farm and asks "How do I get to Galway", the farmer chews the cud for a minute and says "Going to Galway? Well Jaysus if I were you I wouldn't start from here". Roddy, you're answer is correct, but unfortunately unhelpful. The +1 I would give for correctness is negated by the -1 I'd give for unhelpfulness. Have a good one dude :)
Binary Worrier
@Roddy: Thanks for your suggestions, they are really best practices that we should follow, but we also should not expect the all code you are mantaining are so ideal, we have problems here and there, just say "We should..." won't solve the problems, we need to buy into the ROI and Timeline as well, that is the reality.
lz_prgmr
@binary, well, I wouldn't call it /that/ unhelpful ;-) The question as it stands is more like "I need to go from London to New York for a meeting - Which bus should I take?"
Roddy
@Baiyan, I know all about those kind of commercial issues :-( but I feel you don't yet really understand the operation of the code in LongFunction(). And if that's the case, how can you realistically estimate how long it will take to fix a bug in it?
Roddy
@Roddy: "Which bus should I take?" nice one :) +1 sure feckit why not?
Binary Worrier
If the questioner doesn't know the function well enough to know where it returns, no simple mechanical process will help in debugging. It will almost certainly be necessary to understand the function, and refactoring it may well be the fastest way.
David Thornley
Even if you know how this 1000-line function works, but you can expect such long functions probally has multiple exit points, understanding the function won't help me to pinpoint the line where it returns, the question is all about how could I locate that line, as fast as possible.
lz_prgmr
@Baiyan, I'm at a loss to see how knowing which line the function is returning on is going to help you solve the problem. The 'return' itself isn't the error.
Roddy
A: 

I'd say Refactor your code and split the method into smaller methods. A 1000 lines of code is unmaintainable....

Colin
A: 
#define return { TRACE(LINE); return; }

That fixes your problem 4.

As far as the rest goes thats tjust the problem with coding. This is why many systems return more complex errors (such as an HRESULT from a COM object) and/or spam something to the debug stream when a problem occurs.

A 1000 line function should be re-fectored though. As you are seeing a long function proves incredibly hard to maintain.

Edit: Would the following work better than the above?

#define return TRACE(LINE), return

Have had a few drinks so it may well not.

Goz
think about:if(bOk) return true;:(
lz_prgmr
That's a very good point.
Goz
+4  A: 

Is this C or C++?

If C++, create a new class that wraps bool (say CMyBool), that has an automatic cast to bool.

Now have LongFunction return CMyBool (a quick search and replace will change all returns in LongFuntion to "return CMyBool(x)".

Now put a break point in the ctor for CMyBool, the debugger will now stop when CMyBool is created, which will be on the correct return statement in LongFunction.

The automatic cast to bool will stop CMyBool from breaking the code that uses CMyBool.

That will get you over the initial problem, but you larger problem is that LongFunction needs to be refactored.

Hope this helps.

Binary Worrier
Why searching and replacing all return statements in LongFunction()? It is enough to have a CMyBool(bool) constructor and only change the return-value type for LongFunction, isn't it?
SadSido
@SadSido: Yes, forgive me you are correct. It will try to convert the bool to CMyBool and use the ctor that takes a bool. I've been away from C++ for too long :)
Binary Worrier
@Binary: It is really a creative solution, and it works cool with Sadsido's improvement, but I am following concern: We need to change the existing code (although only the return type, but think about we need to support a large code base)BTW, I really like the way to answer the question: give a solution, and point out we should refactoring the function if possible.
lz_prgmr
Binary Worrier
+4  A: 

If your problem is just plain laziness (nothing wrong with that btw), make sure that all return statements in LongFunction are of the form

return(value);

rather than

return value;

(for instance using a regex search-and-replace)

Then, use a slighlty modified preprocessor macro than your original suggestion:

#define return(value) { if (!value) TRACE(__LINE__); return(value); }

...or even

#define return(value) { assert(value); return(value); }

...or whatever you feel is appropriate

Christoffer
yep, good idea.
EvilTeach
assert is not what we want as return failure at some point is legal. I am considering define the macro this way, but really don't want to change the existing code. as you know, has return(value) every well is really ugly.
lz_prgmr
@Baiyan: 1000 LOC for a function is very ugly. You need to weigh the time you are spending avoiding a solid solution against what a refactor would provide. If you are dead-set against a refactor then the only absolutely reliable way is to use a debugger.
ezpz
A: 

You haven't said what platform your on. Depending on the contents of LongFunction the following is how I'd approach this using gdb:

Let's imagine that your file 'f.cc' has the following lines:

1: bool LongFunction () {  /* ... */ }
2:
3: void bar ()
4: {
5:   bool bSuccess = LongFunction ();
6:   assert (bSuccess);
7: }

Here are the steps in gdb:

  1. Add a breakpoint on the same line as the assert: break f.cc:6

  2. Add a condition to that breakpoint for when bSuccess is false: condition 1 (bSuccess==0)

  3. Run your program until that breakpoint is reached

  4. Set a breakpoint at the beginning of the function body: break f.cc:4

  5. Jump back to that location (and it will stop at the breakpoint): jump f.cc:4

  6. Debug the contents of LongFunction to see why it's failing.

The reason I say that it depends on the contents of LongFunction is that if LongFunction reads input from a stream, or modifies global variables etc then its behaviour the second time round may be different. You should consider the above steps as being the same as if the code was:

3: void bar ()
4: {
5:   bool bSuccess = LongFunction ();
5:   bSuccess = LongFunction ();
6:   assert (bSuccess);
7: }
Richard Corden
Richard, let us just assume the behavior won't change if you jump back (anyway,we could re-run the application if not), the keypoint is how should I know in which line in LongFunction() return the failure.
lz_prgmr
@Baiyan: The above technique is helpful in situations where you have a function called successfully many many times, until one call fails. Debugging from the program start may not be an option and this approach solves that. How many returns does the function actually have? An emacs/vi macro could be written where once you reached step 5 in the above - you'd run the macro (setting breakpoints at all return statements in LongFunction) and then you'd continue. This would show you the exact point that the function exists.
Richard Corden
A: 

It visual studio I would place a breakpoint on the assert, then using the stack trace window click the next row up and that'll take you into the exit point of the method.

Antony Koch
Really??? what does "click next row up " means? I remembered we should not see the internal calling of LongFunction in the callstack as you are already out of it.
lz_prgmr
In the stack trace window you should see all the previous calls in the stack - your current call (the assert) should be highlighted, and should have several rows above or below it. You can double click any one of these rows and go to that place in the stack. It's good because you can see all the values as they were at that place on the stack, too, which can be very useful
Antony Koch
I've done this in c# and vb.net btw - I don't know if the same functionality exists in c++ etc. but I would have though it would.
Antony Koch
+9  A: 

Obviously you ought to refactor this function, but in C++ you can use this simple expedient to deal with this in five minutes:

class ReturnMarker
{
   public:
   ReturnMarker()  {};
   ~ReturnMarker() 
   {
      dummy += 1; //<-- put your breakpoint here
   }
   static int dummy;
}

int ReturnMarker::dummy = 0;

and then instance a single ReturnMarker at the top of your function. When it returns, that instance will go out of scope, and you'll hit the destructor.

void LongFunction()
{
    ReturnMarker foo;

    // ...
}
Crashworks
+1, This is cooool, It just cause a minimum impact on existing code.
lz_prgmr
It's an application of the more general idea behind RAII.
Crashworks
Yea, It is used widely. I've thought about this before ask the question, but I only focus on whether I could print out the line num in destructor, didn't notice just set the breakpoint in destructor will work:)
lz_prgmr
Great solution.
sharth
+1 for a good suggestion (and I love RAII), but I'm not sure this will actually do what you want: You will hit the breakpoint on return, but the compiler will probably only emit a single call to the destructor and then JMP there after setting the return value. So, while you hit the breakpoint, you may still have no idea how you got there. This behaviour is likely to be compiler-dependent, so you may get lucky, but I wouldn't bet on it. Also, you'll hit the breakpoint if your function throws an exception, which may be useful to you as well.
Roddy
Usually MSVC does preserve the call stack in this case, or at least it does in debug builds. Making the dummy int static is important though -- if it were local or a class member, the compiler would notice that the variable gets thrown out anyway and probably just elide the destructor altogether.
Crashworks
@Crashworks - You'll get a call stack, but will the actual line with the appropriate 'return' statement be listed, or would it be a generic compiler-generated 'return-and-tidy' at the end of the function? @Baiyan - Did this actually work for you?
Roddy
In MSVC debug builds I definitely get the correct return address. In release it's more of a crapshoot. I don't know what gcc -g -O0 will do.
Crashworks
@Crashworks. I checked it myself using BCB2009, and on a small test case it works a treat!
Roddy
It works on my MSVC2008 in debug mode.
lz_prgmr
A: 

"I could probably debug it step by step, it works but is time-consuming, we don't what to do it this way."

How else do you debug?

I used to debug by:

1) finding a set of parameters that reproduce the problem.

2) step through the code with that set of parameters.

If there are a 1000 lines of code then how are you going to "refactor" without knowing EXACTLY what the function does and is supposed to do.

And how are you going to do this without stepping through the function. I thought that was what an IDE with a good debugger was for.

Quite frankly, I find the question almost humorous in a very sad way.

pgast
-1,I don't think you read the question cafefully, not to mention the comments. I don't want to refactor, I just want to find where cause the function fails in the LongFunction(), is that not you guys do in this situation???
lz_prgmr
I very much read the question and fail to understand how one can avoid doing 1 and 2 if one is actually going to be make a conscientious effort to debug the problem. Debugging problems in bad legacy code is as you say "tedious" and has much "manual work which can't be automated".
pgast
1. I am reproducing the problem 2. I am pretty clear about how the LongFunction() works. But the LongFunction() is too long and have multiple exit point, I want to pinpoint where it fails, that is the question.
lz_prgmr
A: 

In 1982, I was tasked with fixing a broken flowchart program. The program, written in machine-dependent Harris FORTRAN (I do not now recall whether it was in FORTRAN IV or FORTRAN 77), consisted of an 1100-line main program, a 900-line subroutine, and about a dozen subroutines that were each 10 to 20 lines long.

Oh, and there was almost no whitespace (blank lines) in the program, and the comments were not useful at all.

It took me 160 hours - four weeks, FULL TIME, with NOTHING else on my desk - to grok that code sufficiently to make proper repairs.

You are in a similar situation. It is going to take a real investment of CPU time on your part for you to grok that 1000-line creature, well enough to have a handle on what all might go wrong, and what to do to fix it.

Finding the returns is easy enough. For Microsoft AbysmalC++: Search for every instance of "return" and preface it with

"printf("\n\n--->>> punching out at line %d\n\n", __LINE__);"

(or the equivalent on your system). Obviously, you can't just do this automatically; you'll have to look at the local bracketing. Then run your test and see where it tells you to look.

The fact of the matter is this: In the real world of computing, there are almost no real routines out there that actually need to be 1000 lines long. In nearly 40 years in this racket, as student and professional, I have encountered exactly one routine that NEEDED to be longer than one printer page (about 60 lines) long, and that one was a very special case. It was about three pages, all told.

On the other hand, I've seen a LOT of run-on modules, where the guy who wrote it was too lazy, or too incompetent, to factor it properly, and the maintenance programmers were too lazy, too incompetent, or too scared of their manager to go back and refactor it.

Finally: Consider that this is probably not going to be the last time someone has to work on this module, and it may well not be the last time YOU have to work on it. These kinds of things tend to be tar babies: once you touch them, you never get unstuck from them. It might very well be worth your time to refactor at least, and quite possibly redesign/rewrite it FROM SCRATCH and first principles.

John R. Strohm