[lldb-dev] Passing std::atomics by value

[Zachary Turner via lldb-dev]

IOW, marking it =delete() is no different than deleting the copy
constructor above, but at least if you mark it delete, maybe someone will
read the comment above it that explains why it's deleted :)

On Fri, Aug 26, 2016 at 1:13 PM Zachary Turner <zturner at google.com> wrote:

> I think so.  But in this case lldb::Address explicitly supplied a copy
> constructor that looked like this:
>
>     Address (const Address& rhs) :
>         m_section_wp (rhs.m_section_wp),
>         m_offset(rhs.m_offset.load())   // this is the std::atomic<>
>     {
>     }
>
> circumventing the problem.
>
> On Fri, Aug 26, 2016 at 1:11 PM Mehdi Amini <mehdi.amini at apple.com> wrote:
>
>> On Aug 26, 2016, at 1:02 PM, Zachary Turner via lldb-dev <
>> lldb-dev at lists.llvm.org> wrote:
>>
>> It seems to be.  I will also make the copy constructor =delete() to make
>> sure this cannot happen again.
>>
>>
>> Just curious: if a member has a deleted copy-ctor (like std::atomic
>> right?), isn’t the copy constructor automatically deleted?
>>
>> —
>> Mehdi
>>
>>
>>
>> I'm still concerned that the std::atomic is unnecessary, but at that
>> point at least it just becomes a performance problem and not a bug.
>>
>> On Fri, Aug 26, 2016 at 1:00 PM Greg Clayton <gclayton at apple.com> wrote:
>>
>>> So after speaking with local experts on the subject, we do indeed have a
>>> problem. Please convert all placed where we pass lldb_private::Address by
>>> value to pass by "const Address &". Anyone that is modifying the address
>>> should make a local copy and work with that.
>>>
>>> Is Address the only class that is causing problems?
>>>
>>> Greg
>>>
>>> > On Aug 26, 2016, at 10:51 AM, Zachary Turner via lldb-dev <
>>> lldb-dev at lists.llvm.org> wrote:
>>> >
>>> > I recently updated to Visual Studio 2015 Update 3, which has improved
>>> its diagnostics.  As a result of this, LLDB is uncompilable due to a slew
>>> of errors of the following nature:
>>> >
>>> > D:\src\llvm\tools\lldb\include\lldb/Target/Process.h(3256): error
>>> C2719: 'default_stop_addr': formal parameter with requested alignment of 8
>>> won't be aligned
>>> >
>>> > The issue comes down to the fact that lldb::Address contains a
>>> std::atomic<uint64_t>, and is being passed by value pervasively throughout
>>> the codebase.  There is no way to guarantee that this value is 8 byte
>>> aligned.  This has always been a bug, but until now the compiler just
>>> hasn't been reporting it.
>>> >
>>> > Someone correct me if I'm wrong, but I believe this is a problem on
>>> any 32-bit platform, and MSVC is just the only one erroring.
>>> >
>>> > I'm not really sure what to do about this.  Passing
>>> std::atomic<uint64>'s by value seems wrong to me.
>>> >
>>> > Looking at the code, I don't even know why it needs to be atomic.
>>> It's not even being used safely.  We'll have a single function write the
>>> value and later read the value, even though it could have been used in the
>>> meantime.  Maybe what is really intended is a mutex.  Or maybe it doesn't
>>> need to be atomic in the first place.
>>> >
>>> > Does anyone have a suggestion on what to do about this?  I'm currently
>>> blocked on this as I can't compile LLDB.
>>> > _______________________________________________
>>> > lldb-dev mailing list
>>> > lldb-dev at lists.llvm.org
>>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>>>
>>> _______________________________________________
>> lldb-dev mailing list
>> lldb-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20160826/ca46d27b/attachment.html>