| Message ID | CAOyqgcW3Lu4CqTdKveMGO+2dakO8fPcXWziDqt=gs2usQpNV=Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
* 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. (The old code looks like it has a memory leak, so the stack allocation is reasonable anyway.)
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(). 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?
* 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 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. 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). 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)? We probably could add libstdc++ compile-time tests which make sure that all relevant C++ types can be allocated with the global operator new.
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.
* Carlos O'Donell: >> 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. Strictly speaking, that's true. > 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. There is a slight risk that subsequent members will not sufficient alignment if such an alignment was requested explicitly by the programmer. > The broader question is: How do we get rid of the warning? We could add a “please align inside struct, but do not increase the top-most struct alignment” feature to GCC if it does not have one yet. But I don't think it's worth this effort to do this just for HPPA. We should bump MALLOC_ABI_ALIGNMENT and max_align_t and be done with it. I don't see anything else worth doing which also gets rid of the warning. It's also fairly simple conceptually and carries less risk of breaking in unexpected ways later.
On Wed, 2016-09-21 at 16:56 +0200, Florian Weimer wrote: > 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)? I think this should be possible. The int used for the actual lock is first in the structure, followed by two more int's that we could move to the end of the structure (which is just padding currently). That would allow us to shift the lock member by 8 bytes on demand at runtime.
On Wed, 21 Sep 2016, John David Anglin wrote: > 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? The ISO C rule (given the fix to DR#445) is that max_align_t must be at least as aligned as, inter alia, "all types specified in clause 7 as complete object types". The idea is that it must be at least as aligned as any type the user can define using only standard features other than _Alignas. It would seem reasonable for it to be at least as aligned as POSIX types as well, in cases where POSIX types have greater alignment requirements than ISO C types; clearly malloc needs to return memory sufficiently aligned for POSIX types (because POSIX inherits from C99, where malloc has the "any type of object" wording). It's vector types we want to avoid contributing to max_align_t alignment.
Index: gcc/go/gofrontend/MERGE =================================================================== --- gcc/go/gofrontend/MERGE (revision 240275) +++ gcc/go/gofrontend/MERGE (working copy) @@ -1,4 +1,4 @@ -80720773ac1a3433b7de59ffa5c04744123247c3 +57d120d75be87c2a0da67e750f16929891f1b8f4 The first line of this file holds the git revision number of the last merge done from the gofrontend repository. Index: gcc/go/gofrontend/ast-dump.cc =================================================================== --- gcc/go/gofrontend/ast-dump.cc (revision 240053) +++ gcc/go/gofrontend/ast-dump.cc (working copy) @@ -166,24 +166,24 @@ const char* kAstDumpFileExtension = ".du void Ast_dump_context::dump(Gogo* gogo, const char* basename) { - std::ofstream* out = new std::ofstream(); + std::ofstream out; std::string dumpname(basename); dumpname += ".dump.ast"; - out->open(dumpname.c_str()); + out.open(dumpname.c_str()); - if (out->fail()) + if (out.fail()) { error("cannot open %s:%m, -fgo-dump-ast ignored", dumpname.c_str()); return; } this->gogo_ = gogo; - this->ostream_ = out; + this->ostream_ = &out; Ast_dump_traverse_blocks_and_functions adtbf(this); gogo->traverse(&adtbf); - out->close(); + out.close(); } // Dump a textual representation of a type to the
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. Ian