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

[Greg Clayton via lldb-dev]

> On Aug 29, 2016, at 8:56 AM, Zachary Turner  wrote:
> 
> How about making it a std::unique_ptr?  This way 
> there's no risk of re-introducing a potential race, and copying still works.

lldb_private::Address is a member variable in lldb_private::Symbol and it needs 
to say as small as possible as lldb_private::Symbol is the number one memory 
hog in LLDB. Adding a unique pointer would make each address now have two 
memory entries: one for the Address (on the stack or heap) and one for the 
std::atomic on the heap. Address needs to say simple. I agree with you that we 
should rely upon mutexes to take care of threading issues and that Address 
probably shouldn't have to use std::atomic. Lets try and get rid of the atomic 
and fix the code that was having the race by using appropriate threading 
constructs.

Greg


___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Zachary Turner via lldb-dev]

How about making it a std::unique_ptr?  This way
there's no risk of re-introducing a potential race, and copying still works.

On Mon, Aug 29, 2016 at 8:49 AM Zachary Turner  wrote:

> My concern is that by not taking out the copy constructor, someone passes
> another Address by value later. If passing by value is unsafe (ignoring
> whether it generates a compiler error) it should be prevented by the
> compiler. That is doing due diligence.
>
> I will track down the commit that introduced this, but again, all you have
> to do is inspect the class to see that it is not thread aware at all.
> Anyone attempting to share an Address on multiple threads had better be
> using a mutex or they're already gonna have a bad time, in which case the
> atomic seems unnecessary
>
>
>
> On Mon, Aug 29, 2016 at 8:38 AM Greg Clayton  wrote:
>
>>
>> > On Aug 26, 2016, at 5:37 PM, Zachary Turner  wrote:
>> >
>> > 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.
>>
>> It just won't compile on platforms that have this issue. We are not
>> taking the Address copy constructor out just so we can enforce not passing
>> this object by value, that is not a viable solution. The copy constructor
>> of address currently works around this issue, but the solution is not to
>> take out the copy constructor of Address.
>>
>> > BTW, nobody has commented on my original concern that the atomic may
>> not even be necessary in the first place.
>>
>> Track down the original author and see what the commit message said. I
>> can see how this could cause problems, but maybe if we don't allow Address
>> to be passed by value the problem goes away. I am not sure. Just taking it
>> out because it doesn't compile in your platform is not the way forward
>> without doing due diligence.
>>
>> So just change the places where this gets passed by value and be done
>> with it, or take the time to make sure you don't introduce a threading
>> issue and decide to take out the std::atomic.
>>
>> Greg Clayton
>>
>>
>>
>>
>>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Greg Clayton via lldb-dev]

> On Aug 26, 2016, at 5:37 PM, Zachary Turner  wrote:
> 
> 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.

It just won't compile on platforms that have this issue. We are not taking the 
Address copy constructor out just so we can enforce not passing this object by 
value, that is not a viable solution. The copy constructor of address currently 
works around this issue, but the solution is not to take out the copy 
constructor of Address. 

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

Track down the original author and see what the commit message said. I can see 
how this could cause problems, but maybe if we don't allow Address to be passed 
by value the problem goes away. I am not sure. Just taking it out because it 
doesn't compile in your platform is not the way forward without doing due 
diligence. 

So just change the places where this gets passed by value and be done with it, 
or take the time to make sure you don't introduce a threading issue and decide 
to take out the std::atomic.

Greg Clayton




___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [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  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  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 
> 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 
> wrote:
> >> On Aug 26, 2016, at 1:02 PM, Zachary Turner via lldb-dev <
> lldb-dev@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 
> 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@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, 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'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@lists.llvm.org
> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> >>
> >> ___
> >> lldb-dev mailing list
> >> lldb-dev@lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Greg Clayton via lldb-dev]

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  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  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  wrote:
>> On Aug 26, 2016, at 1:02 PM, Zachary Turner via lldb-dev 
>>  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  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 
>> >  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, 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'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@lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>> 
>> ___
>> lldb-dev mailing list
>> lldb-dev@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Mehdi Amini via lldb-dev]

> On Aug 26, 2016, at 1:13 PM, Zachary Turner  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 :)

Got it, make sense.

Thanks.

— 
Mehdi


> 
> On Fri, Aug 26, 2016 at 1:13 PM Zachary Turner  > 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  > wrote:
>> On Aug 26, 2016, at 1:02 PM, Zachary Turner via lldb-dev 
>> > 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 > > 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 
>> > > 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, 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'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@lists.llvm.org 
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev 
>> > 
>> 
>> ___
>> lldb-dev mailing list
>> lldb-dev@lists.llvm.org 
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev 
>> 

___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [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  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  wrote:
>
>> On Aug 26, 2016, at 1:02 PM, Zachary Turner via lldb-dev <
>> lldb-dev@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  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@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, 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'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@lists.llvm.org
>>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>>>
>>> ___
>> lldb-dev mailing list
>> lldb-dev@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>>
>>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Zachary Turner via lldb-dev]

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  wrote:

> On Aug 26, 2016, at 1:02 PM, Zachary Turner via lldb-dev <
> lldb-dev@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  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@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, 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'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@lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>>
>> ___
> lldb-dev mailing list
> lldb-dev@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Mehdi Amini via lldb-dev]

> On Aug 26, 2016, at 1:02 PM, Zachary Turner via lldb-dev 
>  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  > 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 
> > > 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, 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'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@lists.llvm.org 
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev 
> > 
> 
> ___
> lldb-dev mailing list
> lldb-dev@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Zachary Turner via lldb-dev]

It seems to be.  I will also make the copy constructor =delete() to make
sure this cannot happen again.

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  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@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, 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'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@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Zachary Turner via lldb-dev]

Incidentally, it seems copy and assignment of std::atomics are deleted
functions, so the C++ standard is telling you you're not really supposed to
do this.

I honestly don't believe this should be a std::atomic at all.

On Fri, Aug 26, 2016 at 12:54 PM Ted Woodward via lldb-dev <
lldb-dev@lists.llvm.org> wrote:

>
> Would the misalignment really be a problem on x86? I thought the core
> handled misaligned loads and stores. Unlike, say, PPC.
>
> Either way, I'd expect the compiler to automatically align a uint64.
>
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project
>
>
> -Original Message-
> From: lldb-dev [mailto:lldb-dev-boun...@lists.llvm.org] On Behalf Of Jim
> Ingham via lldb-dev
> Sent: Friday, August 26, 2016 2:32 PM
> To: Zachary Turner <ztur...@google.com>
> Cc: LLDB <lldb-dev@lists.llvm.org>
> Subject: Re: [lldb-dev] Passing std::atomics by value
>
> That seems to me a compiler bug, not: "You can't pass structures with
> std::atomic" elements in them by value.  It would crazy to add a type to
> the C++ language that you couldn't use everywhere.  There doesn't seem to
> be any official restriction.  And anecdotally there's a reference to the
> problem in gcc for PPC (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259)
> but that was considered a bug and fixed.
>
> Not that it matters much, you are stuck with the compiler you're stuck
> with, bugs and all.
>
> Jim
>
> > On Aug 26, 2016, at 12:26 PM, Zachary Turner <ztur...@google.com> wrote:
> >
> > It could, in theory, it just doesn't.  I compiled a quick program using
> i686-darwin as the target and the generated assembly makes no attempt to
> pad the arguments.
> >
> > I'll post some code later
> >
> > On Fri, Aug 26, 2016 at 11:42 AM Jim Ingham <jing...@apple.com> wrote:
> >
> > > On Aug 26, 2016, at 11:36 AM, Zachary Turner via lldb-dev <
> lldb-dev@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.
> >
> > Why can't the compiler pad the argument slot on the stack so that the
> actual struct lives at a properly aligned location?  It's copying the value
> into the callee's stack frame, so it can put it wherever it wants.  And
> both caller and callee sites know the alignment requirements from the
> function signature, so they can both figure out where the actual struct
> lives in the argument slot based on the alignment of the stack slot.
> >
> > Jim
> >
> >
> > >
> > > On Fri, Aug 26, 2016 at 11:29 AM Greg Clayton <gclay...@apple.com>
> wrote:
> > >
> > > > On Aug 26, 2016, at 11:24 AM, Greg Clayton via lldb-dev <
> lldb-dev@lists.llvm.org> wrote:
> > > >
> > > >>
> > > >> On Aug 26, 2016, at 10:51 AM, Zachary Turner via lldb-dev <
> lldb-dev@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, 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'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 valu

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

[Ted Woodward via lldb-dev]

Would the misalignment really be a problem on x86? I thought the core handled 
misaligned loads and stores. Unlike, say, PPC.

Either way, I'd expect the compiler to automatically align a uint64.

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project


-Original Message-
From: lldb-dev [mailto:lldb-dev-boun...@lists.llvm.org] On Behalf Of Jim Ingham 
via lldb-dev
Sent: Friday, August 26, 2016 2:32 PM
To: Zachary Turner <ztur...@google.com>
Cc: LLDB <lldb-dev@lists.llvm.org>
Subject: Re: [lldb-dev] Passing std::atomics by value

That seems to me a compiler bug, not: "You can't pass structures with 
std::atomic" elements in them by value.  It would crazy to add a type to the 
C++ language that you couldn't use everywhere.  There doesn't seem to be any 
official restriction.  And anecdotally there's a reference to the problem in 
gcc for PPC (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259) but that was 
considered a bug and fixed.

Not that it matters much, you are stuck with the compiler you're stuck with, 
bugs and all.

Jim

> On Aug 26, 2016, at 12:26 PM, Zachary Turner <ztur...@google.com> wrote:
> 
> It could, in theory, it just doesn't.  I compiled a quick program using 
> i686-darwin as the target and the generated assembly makes no attempt to pad 
> the arguments.
> 
> I'll post some code later
> 
> On Fri, Aug 26, 2016 at 11:42 AM Jim Ingham <jing...@apple.com> wrote:
> 
> > On Aug 26, 2016, at 11:36 AM, Zachary Turner via lldb-dev 
> > <lldb-dev@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.
> 
> Why can't the compiler pad the argument slot on the stack so that the actual 
> struct lives at a properly aligned location?  It's copying the value into the 
> callee's stack frame, so it can put it wherever it wants.  And both caller 
> and callee sites know the alignment requirements from the function signature, 
> so they can both figure out where the actual struct lives in the argument 
> slot based on the alignment of the stack slot.
> 
> Jim
> 
> 
> >
> > On Fri, Aug 26, 2016 at 11:29 AM Greg Clayton <gclay...@apple.com> wrote:
> >
> > > On Aug 26, 2016, at 11:24 AM, Greg Clayton via lldb-dev 
> > > <lldb-dev@lists.llvm.org> wrote:
> > >
> > >>
> > >> On Aug 26, 2016, at 10:51 AM, Zachary Turner via lldb-dev 
> > >> <lldb-dev@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, 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'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.
>

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

[Jim Ingham via lldb-dev]

That seems to me a compiler bug, not: "You can't pass structures with 
std::atomic" elements in them by value.  It would crazy to add a type to the 
C++ language that you couldn't use everywhere.  There doesn't seem to be any 
official restriction.  And anecdotally there's a reference to the problem in 
gcc for PPC (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62259) but that was 
considered a bug and fixed.

Not that it matters much, you are stuck with the compiler you're stuck with, 
bugs and all.

Jim

> On Aug 26, 2016, at 12:26 PM, Zachary Turner  wrote:
> 
> It could, in theory, it just doesn't.  I compiled a quick program using 
> i686-darwin as the target and the generated assembly makes no attempt to pad 
> the arguments.
> 
> I'll post some code later
> 
> On Fri, Aug 26, 2016 at 11:42 AM Jim Ingham  wrote:
> 
> > On Aug 26, 2016, at 11:36 AM, Zachary Turner via lldb-dev 
> >  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.
> 
> Why can't the compiler pad the argument slot on the stack so that the actual 
> struct lives at a properly aligned location?  It's copying the value into the 
> callee's stack frame, so it can put it wherever it wants.  And both caller 
> and callee sites know the alignment requirements from the function signature, 
> so they can both figure out where the actual struct lives in the argument 
> slot based on the alignment of the stack slot.
> 
> Jim
> 
> 
> >
> > On Fri, Aug 26, 2016 at 11:29 AM Greg Clayton  wrote:
> >
> > > On Aug 26, 2016, at 11:24 AM, Greg Clayton via lldb-dev 
> > >  wrote:
> > >
> > >>
> > >> On Aug 26, 2016, at 10:51 AM, Zachary Turner via lldb-dev 
> > >>  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, 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'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@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> 

___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Zachary Turner via lldb-dev]

It could, in theory, it just doesn't. I compiled a quick program using
i686-darwin as the target and the generated assembly makes no attempt to
pad the arguments.

I'll post some code later

On Fri, Aug 26, 2016 at 11:42 AM Jim Ingham  wrote:

>
> > On Aug 26, 2016, at 11:36 AM, Zachary Turner via lldb-dev <
> lldb-dev@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.
>
> Why can't the compiler pad the argument slot on the stack so that the
> actual struct lives at a properly aligned location?  It's copying the value
> into the callee's stack frame, so it can put it wherever it wants.  And
> both caller and callee sites know the alignment requirements from the
> function signature, so they can both figure out where the actual struct
> lives in the argument slot based on the alignment of the stack slot.
>
> Jim
>
>
> >
> > On Fri, Aug 26, 2016 at 11:29 AM Greg Clayton 
> wrote:
> >
> > > On Aug 26, 2016, at 11:24 AM, Greg Clayton via lldb-dev <
> lldb-dev@lists.llvm.org> wrote:
> > >
> > >>
> > >> On Aug 26, 2016, at 10:51 AM, Zachary Turner via lldb-dev <
> lldb-dev@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, 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'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@lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Jim Ingham via lldb-dev]

> On Aug 26, 2016, at 11:44 AM, Adrian McCarthy via lldb-dev 
>  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 
>  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  wrote:
> 
> > On Aug 26, 2016, at 11:24 AM, Greg Clayton via lldb-dev 
> >  wrote:
> >
> >>
> >> On Aug 26, 2016, at 10:51 AM, Zachary Turner via lldb-dev 
> >>  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, 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'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@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> 
> 
> ___
> lldb-dev mailing list
> lldb-dev@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [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@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  wrote:
>
>>
>> > On Aug 26, 2016, at 11:24 AM, Greg Clayton via lldb-dev <
>> lldb-dev@lists.llvm.org> wrote:
>> >
>> >>
>> >> On Aug 26, 2016, at 10:51 AM, Zachary Turner via lldb-dev <
>> lldb-dev@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, 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'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@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
>
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Jim Ingham via lldb-dev]

> On Aug 26, 2016, at 11:36 AM, Zachary Turner via lldb-dev 
>  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.

Why can't the compiler pad the argument slot on the stack so that the actual 
struct lives at a properly aligned location?  It's copying the value into the 
callee's stack frame, so it can put it wherever it wants.  And both caller and 
callee sites know the alignment requirements from the function signature, so 
they can both figure out where the actual struct lives in the argument slot 
based on the alignment of the stack slot.

Jim


> 
> On Fri, Aug 26, 2016 at 11:29 AM Greg Clayton  wrote:
> 
> > On Aug 26, 2016, at 11:24 AM, Greg Clayton via lldb-dev 
> >  wrote:
> >
> >>
> >> On Aug 26, 2016, at 10:51 AM, Zachary Turner via lldb-dev 
> >>  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, 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'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@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Zachary Turner via lldb-dev]

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  wrote:

>
> > On Aug 26, 2016, at 11:24 AM, Greg Clayton via lldb-dev <
> lldb-dev@lists.llvm.org> wrote:
> >
> >>
> >> On Aug 26, 2016, at 10:51 AM, Zachary Turner via lldb-dev <
> lldb-dev@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, 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'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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Greg Clayton via lldb-dev]

> On Aug 26, 2016, at 11:24 AM, Greg Clayton via lldb-dev 
>  wrote:
> 
>> 
>> On Aug 26, 2016, at 10:51 AM, Zachary Turner via lldb-dev 
>>  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, 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'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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

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

[Zachary Turner via lldb-dev]

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, 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'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@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev