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

[Adrian McCarthy via lldb-dev]

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.

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20160826/e3af8298/attachment.html>