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

Fri Aug 26 11:52:22 PDT 2016

> On Aug 26, 2016, at 11:44 AM, Adrian McCarthy via lldb-dev <lldb-dev at lists.llvm.org> wrote:
> 
> Methods like Address::SetOffset and Address::Slide seem to have data races despite m_offset being atomic.  Callers of those methods would have to guarantee that nothing else is trying to write to m_offset.  And if they're doing that, then there doesn't appear to be a need for std::atomic on that field.
> 
> BTW, I propose we move the member variables from protected to private.  As far as I can tell, there aren't any derived classes (yet), at least none that access those members.  If we need to add a mutex to the class itself, it'll be better if any future derived classes access the data through accessors.

That seems fine to me.  We haven't needed to subclass it in years, and probably won't.

Jim

> 
> On Fri, Aug 26, 2016 at 11:36 AM, Zachary Turner via lldb-dev <lldb-dev at lists.llvm.org> wrote:
> The thing is, the code is already full of data races.  I think the std::atomic is actually used incorrectly and is not even doing anything.
> 
> That said, consider darwin on 32-bit, where I believe each stack frame is 4-byte aligned.  I dont' think there's any way the compiler can guarantee that a function parameter is 8-byte aligned without allocating from the heap, which is obviously impossible for a stack variable.
> 
> On Fri, Aug 26, 2016 at 11:29 AM Greg Clayton <gclayton at apple.com> wrote:
> 
> > On Aug 26, 2016, at 11:24 AM, Greg Clayton via lldb-dev <lldb-dev at lists.llvm.org> wrote:
> >
> >>
> >> 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.
> >
> > Feel free to #ifdef around the m_offset member of Address and you can have the data race and compile. This seems like a compiler bug to me. If a struct/classe by value argument has alignment requirements, then the compiler should handle this correctly IMHO. Am I wrong????
> 
> Or if this isn't a compiler bug, feel free to modify anything that was passing Address by value and make it a "const Address &".
> 
> _______________________________________________
> 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