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

[Zachary Turner via lldb-dev]

How would you enforce that, other than to ask people to try to remember not
to do it?  It seems to me like std::atomic not being copy-constructible is
telling you that, well, you shouldn't be copying it.

BTW, nobody has commented on my original concern that the atomic may not
even be necessary in the first place.

On Fri, Aug 26, 2016 at 5:13 PM Greg Clayton <gclayton at apple.com> wrote:

> There is no need to delete the lldb_private::Address copy constructor.
> Just stop functions from passing it by value.
>
> > On Aug 26, 2016, at 1:13 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > 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/20160827/eda56cf3/attachment-0001.html>