[lldb-dev] 32-bit linux IsWatchpointHit assertion failure due to WriteRegister

[Todd Fiala]

tfiala at tfiala2:/mnt/ssd/work/svn/lgs/llvm/tools/lldb$ svn commit
Sending        source/Plugins/Process/Linux/ProcessMonitor.cpp
Sending        source/Plugins/Process/POSIX/RegisterContextLinux_i386.cpp
Transmitting file data ..
Committed revision 202887


On Tue, Mar 4, 2014 at 10:33 AM, Todd Fiala <tfiala at google.com> wrote:

> Thanks Matt.
>
> Right now I'm going to try what I think are the last two patches you had
> put up since we last chatted (my last night).
>
>
> On Tue, Mar 4, 2014 at 12:47 AM, Matthew Gardiner <mg11 at csr.com> wrote:
>
>> Todd Fiala wrote:
>>
>>> So I tried this patch:
>>>
>>> tfiala at ubuntu:~/lldb/git/llvm/tools/lldb/source/Plugins$ git diff
>>> diff --git a/source/Plugins/Process/POSIX/RegisterContextLinux_i386.cpp
>>> b/source
>>> index f2c1bab..72f8838 100644
>>> --- a/source/Plugins/Process/POSIX/RegisterContextLinux_i386.cpp
>>> +++ b/source/Plugins/Process/POSIX/RegisterContextLinux_i386.cpp
>>> @@ -83,8 +83,10 @@ struct UserArea
>>>  };
>>>  #define DR_SIZE sizeof(UserArea::u_debugreg[0])
>>> -#define DR_OFFSET(reg_index) \
>>> -    (LLVM_EXTENSION offsetof(UserArea, u_debugreg[reg_index]))
>>> +
>>> +// FIXME: remove the following code as soon as we get the UserArea
>>> structure fi
>>> +#define DR_OFFSET(reg_index) (0xFC + (reg_index * 4))
>>> +// #define DR_OFFSET(reg_index)        (LLVM_EXTENSION
>>> offsetof(UserArea, u_deb
>>>  #define FPR_SIZE(reg) sizeof(((FPR_i386*)NULL)->reg)
>>>  //----------------------------------------------------------
>>> -----------------
>>>
>>> But I'm still getting the assertion.  I did put an #error in the file to
>>> make sure I wasn't somehow not building it.
>>>
>> Yes. That's a different (at least that's how I see it) assertion. It
>> occurs because:
>>
>> bool
>> RegisterContextPOSIXProcessMonitor_x86_64::IsWatchpointHit(uint32_t
>> hw_index)
>> {
>> <snip>
>>
>>         RegisterValue zero_bits = RegisterValue(uint64_t(0));
>>         if (!WriteRegister(m_reg_info.first_dr + 6, zero_bits) ||
>> !WriteRegister(m_reg_info.first_dr + 7, zero_bits))
>>
>> doesn't write 0. It writes 0xffffffff. Which results in all breakpoint
>> indexes being set but no watchpoints in lldb structures, hence the assert.
>>
>> That's due to a combination of:
>>
>> void
>> WriteRegOperation::Execute(ProcessMonitor *monitor)
>> {
>> <snip>
>> #if __WORDSIZE == 32
>>     buf = (void*) m_value.GetAsUInt32();
>> #else
>>     buf = (void*) m_value.GetAsUInt64();
>> #endif
>>
>> and
>>
>> uint32_t
>> RegisterValue::GetAsUInt32 (uint32_t fail_value, bool *success_ptr) const
>> {
>> <snip>
>>     switch (m_type)
>>     {
>>         default:            break;
>>         case eTypeUInt8:    return m_data.uint8;
>>         case eTypeUInt16:   return m_data.uint16;
>>         case eTypeUInt32:   return m_data.uint32;
>>         case eTypeFloat:
>>             if (sizeof(float) == sizeof(uint32_t))
>>                 return m_data.uint32;
>>             break;
>> <snip>
>>     return fail_value;
>> }
>>
>> but I have a sweet fix for this, which doesn't compromise the design of
>> RegisterValue. With this fix and the correct i386 register map, I think
>> we're looking good for 32-bit linux.
>>
>>
>>
>>> I see where my wires crossed when reading the user.h header.
>>>
>> No sweat ;-)
>>
>>  On your question re: copying the structure:
>>> this code has to compile in places other than i386 Linux (i.e. we can
>>> debug i386 Linux code on other hosts, that don't have the user.h available,
>>> or have the wrong one available).
>>>
>> Yeah, I appreciate that. You can't rely on the presence of the "right"
>> user.h. To calculate the offsets you'd need to either i) copy the
>> structures or ii) hard-code the offsets. My contention isn't over the
>> *copying*, it's over the usage of structures, when they're not also being
>> used as structures. I think if the actual offsets e.g.
>>
>> ...
>> const uint32_t kDebugRegister6_Offset = 0x114;
>> const uint32_t kDebugRegister7_Offset = 0x118;
>> ...
>>
>> are crafted, then we have more robust code, since breakage would require
>> people deliberately changing a number, instead of a more subtle structure
>> edit. Anyway, that's just my opinion.
>>
>> thanks
>> Matt
>>
>>
>>
>>
>> Member of the CSR plc group of companies. CSR plc registered in England
>> and Wales, registered number 4187346, registered office Churchill House,
>> Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
>> More information can be found at www.csr.com. Keep up to date with CSR
>> on our technical blog, www.csr.com/blog, CSR people blog,
>> www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook,
>> www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at
>> www.twitter.com/CSR_plc.
>> New for 2014, you can now access the wide range of products powered by
>> aptX at www.aptx.com.
>>
>
>
>
> --
> Todd Fiala | Software Engineer |  tfiala at google.com |  650-943-3180
>



-- 
Todd Fiala | Software Engineer | tfiala at google.com | 650-943-3180
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140304/b02c1fb3/attachment.html>