Re: [gofrontend-dev] Go patch committed: Avoid warning by using a local var for std::ofstream

[Carlos O'Donell <carlos at redhat dot com>]

On 09/21/2016 10:56 AM, Florian Weimer wrote:
> * John David Anglin:
> 
>> On Tue, Sep 20, 2016 at 09:27:17PM +0200, Florian Weimer wrote:
>>> * Ian Lance Taylor:
>>>
>>>> GCC PR 77625 is about a warning while compiling the Go frontend.  The
>>>> Go frontend called `new std::ofstream()`, and the warning is "error:
>>>> `new' of type `std::ofstream {aka std::basic_ofstream<char>}' with
>>>> extended alignment 16".  Frankly I'm not sure how this supposed to
>>>> work: shouldn't it be possible to write new std::ofstream()?  But the
>>>> problem is easy to avoid, since in this case the std::ofstream can be
>>>> a local variable, and that is a better approach anyhow.  This patch
>>>> was bootstrapped on x86_64-pc-linux-gnu.  Committed to mainline.
>>>
>>> This happens only on hppa, right?  It looks as if libstdc++ is
>>> seriously broken there.
>>
>> I'm not sure I understand the comment.  I created a short testcase.
>> I agree that the issue is hppa specific but the extended alignment
>> is coming from the __lock field in pthread_mutex_t.  I'm not sure
>> why this affects new std::ofstream().
> 
> Ahh, thanks for the explanation.  The C++ streams probably have some
> internal locks (hidden behind libstdc++ abstractions).

The alignment is a historical ABI accident that we inherited from
Linuxthreads.

>> The alignment of 16 arises in code that used the ldcw instruction.
>> Although this could be avoided in glibc there are numerous other
>> packages with objects requiring 16-byte alignment.  So, I'm tending
>> to think the typedef for max_align_t should be adjusted on hppa-linux
>> so that it has 16-byte alignment.  Is that the correct approach?
> 
> Yes, and for the warning to go away, GCC's MALLOC_ABI_ALIGNMENT needs
> to be increased as well, I think.

This is not required.

The __attribute__((__aligned__(16))); is not required for pthread locks
to work correctly, it is simply there to ensure structure padding and
layout matches the pre-NPTL ABI to ensure we don't break ABI.

In the original Linuxthreads code there was indeed a singular lock word
that needed 16-byte alignment because the hppa 'load and clear word'
atomic operation needed that alignment.

In NPTL the alignment restrictions are lifted and we use a light-weight
kernel helper (like ARM and m68k) which does compare-and-swap atomically
using kernel-side locks (synchronized with futex operations in the
kernel).

So the alignment is only needed for structure layout.

Malloc can return word aligned memory for a pthread_mutex_t and it would
work just fine.

The broader question is: How do we get rid of the warning?

> Nowadays, we can change the glibc malloc alignment fairly easily, but
> interposed mallocs won't be so nice.  But it is unlikely that any of
> this matters in practice because the issue is not new (only the
> warning is).

The issue is not new, but changing glibc's malloc alignment is not
required.

> I have Cc:ed a few people who might have ideas how to deal with this.
> 
> Torvald, would it be possible to align mutexes internally on hppa, to
> avoid the 16-byte alignment of the entire struct (that is, store a
> pointer to the actual mutex object, which points to a sub-region of
> the struct which is suitably aligned)?

The attribute aligned is used for laying out larger structures where the
pthread structures are embedded in them. Therefore removing attribute
aligned would result in an ABI break for all such embedded structures.
The attribute aligned must remain for such structures to continue to be
laid out the same way they were with Linuxthreads, and now NPTL.

As I mentioned before, the alignment is not required for the correct
operation of the locks.

> We probably could add libstdc++ compile-time tests which make sure
> that all relevant C++ types can be allocated with the global operator
> new.

So how do we get rid of the warning?

We need to keep __attribute__((__aligned(16))); because of structure
layout ABI compatibility, but we don't actually need the alignment at
runtime anymore.

-- 
Cheers,
Carlos.