[lldb-dev] TestRecursiveInferior.py

[Thirumurthi, Ashok]

Thanks Jason, all good points.  

I removed the code block quoted below from UnwindLLDB (see r191430) since it's neither correct, complete nor reachable.  The code block from RegisterContextLLDB that you quoted got me wondering how to test for this stack setup.  

Also, if we can oscillate between two CFAs, then wouldn't any size of cycle be possible?  Perhaps the more robust defense is to stop the unwind if the new frame's CFA exists anywhere in the thread's call stack...

- Ashok

-----Original Message-----
From: Jason Molenda [mailto:jmolenda at apple.com] 
Sent: Wednesday, September 25, 2013 6:09 PM
To: Thirumurthi, Ashok
Cc: Greg Clayton; lldb-dev at cs.uiuc.edu
Subject: Re: [lldb-dev] TestRecursiveInferior.py

Yes, in this code segment

    if (!m_frames.empty())
    {
        if (m_frames.back()->start_pc == cursor_sp->start_pc)
        {
            if (m_frames.back()->cfa == cursor_sp->cfa)
                goto unwind_done; // Infinite loop where the current cursor is the same as the previous one...
            else if (abi && abi->StackUsesFrames())
            {
                // We might have a CFA that is not using the frame pointer and
                // we want to validate that the frame pointer is valid.
                if (reg_ctx_sp->GetFP() == 0)
                    goto unwind_done;
            }
        }
    }

I agree that the reg_ctx_sp->GetFP() == 0 check is incorrect.  This assumes that the code is using the fp register as a frame pointer (instead of as a gpr).  A canonical frame address (CFA) of 0 obviously means that a stack walk should stop, but the CFA is not necessarily set in terms of the fp register (e.g. in -fomit-frame-pointer style code).

This code should be removed.

I'm not sure the m_frames.back()->cfa == cursor_sp->cfa bit is doing anything either.  Over in RegisterContextLLDB::InitializeNonZerothFrame() 

    // A couple of sanity checks..
    if (cfa_regval == LLDB_INVALID_ADDRESS || cfa_regval == 0 || cfa_regval == 1)
    {
        UnwindLogMsg ("could not find a valid cfa address");
        m_frame_type = eNotAValidFrame;
        return;
    }

    // If we have a bad stack setup, we can get the same CFA value multiple times -- or even
    // more devious, we can actually oscillate between two CFA values.  Detect that here and
    // break out to avoid a possible infinite loop in lldb trying to unwind the stack.
    addr_t next_frame_cfa;
    addr_t next_next_frame_cfa = LLDB_INVALID_ADDRESS;
    if (GetNextFrame().get() && GetNextFrame()->GetCFA(next_frame_cfa))
    {
        bool repeating_frames = false;
        if (next_frame_cfa == m_cfa)
        {
            repeating_frames = true;
        }
        else
        {
            if (GetNextFrame()->GetNextFrame() && GetNextFrame()->GetNextFrame()->GetCFA(next_next_frame_cfa)
                && next_next_frame_cfa == m_cfa)
            {
                repeating_frames = true;
            }
        }
        if (repeating_frames && abi->FunctionCallsChangeCFA())
        {
            UnwindLogMsg ("same CFA address as next frame, assuming the unwind is looping - stopping");
            m_frame_type = eNotAValidFrame;



On Sep 25, 2013, at 2:28 PM, Thirumurthi, Ashok <ashok.thirumurthi at intel.com> wrote:

> Hi Greg,
>  
> I got around to looking at pr15415 today, and noticed that TestRecursiveInferior.py is a case where r132021 is an issue.  Specifically, this code looks at the case where two subsequent frames have the same pc.  It correctly distinguishes between recursion and an infinite loop by testing for unique canonical frame addresses.  However, it then goes on to assume that the unwind should stop if GetFP() shows a null frame pointer.  However, recursive_inferior/Makefile can achieve this using -fomit-frame-pointer.
>  
> The attached patch fixes the test by nixing the code block that I can't explain (the commit message for r132021 didn't specify why the logic changed for UnwindLLDB, and I can't see any regressions locally with the attached patch, and r132021 doesn't add or fix any test cases).  Do you figure that a stronger test is required to distinguish between a good unwind and a bad one?  If so, what would be required to create a test case for the existing code?
>  
> Note also that overloads of StackUsesFrames() in trunk always return true.  Cheers,
>  
> -   Ashok