Re: [PATCH] Implement new hook for max_align_t_align
On 2017-02-25, at 5:32 PM, Gerald Pfeifer wrote: > On Sat, 25 Feb 2017, John David Anglin wrote: >>> Sooo, why not deprecate 32-bit HP-UX with GCC 7? >> GCC 7 is in reasonable shape on 32-bit HP-UX. I'll do it after it is >> released. > > If you deprecate it with GCC 7, it'd be only removed with GCC 8. > > On the other hand, if you deprecate it past GCC 7, removal would > happen with GCC 9 (by default). Exactly. I'm happy with that and don't want to require --enable-obsolete to build GCC 7. Dave -- John David Anglin dave.ang...@bell.net
Re: [PATCH] Implement new hook for max_align_t_align
On Sat, 25 Feb 2017, John David Anglin wrote: >> Sooo, why not deprecate 32-bit HP-UX with GCC 7? > GCC 7 is in reasonable shape on 32-bit HP-UX. I'll do it after it is > released. If you deprecate it with GCC 7, it'd be only removed with GCC 8. On the other hand, if you deprecate it past GCC 7, removal would happen with GCC 9 (by default). Gerald
Re: [PATCH] Implement new hook for max_align_t_align
On 2017-02-25, at 12:25 PM, Gerald Pfeifer wrote: > On Wed, 12 Oct 2016, John David Anglin wrote: >>> It's really hard to make an argument to do anything other than deprecate >>> that platform. Given John's ongoing involvement it's much harder to >>> deprecate 64bit linux (and consequently 64bit hpux). >> I believe that deprecating the 32-bit HP-UX platform makes sense. There >> is no HP involvement in hppa, ia64 or alpha open source that I am aware >> of, so deprecating gcc platforms with HP systems is reasonable. >> >> My primary focus is open source and one less platform will free some time. > > Sooo, why not deprecate 32-bit HP-UX with GCC 7? GCC 7 is in reasonable shape on 32-bit HP-UX. I'll do it after it is released. Dave -- John David Anglin dave.ang...@bell.net
Re: [PATCH] Implement new hook for max_align_t_align
On Wed, 12 Oct 2016, John David Anglin wrote: >> It's really hard to make an argument to do anything other than deprecate >> that platform. Given John's ongoing involvement it's much harder to >> deprecate 64bit linux (and consequently 64bit hpux). > I believe that deprecating the 32-bit HP-UX platform makes sense. There > is no HP involvement in hppa, ia64 or alpha open source that I am aware > of, so deprecating gcc platforms with HP systems is reasonable. > > My primary focus is open source and one less platform will free some time. Sooo, why not deprecate 32-bit HP-UX with GCC 7? Now is still a good time doing so, from the release perspective, and if you as the maintainer (and pretty much only person active here) are in favor... Gerald
Re: [PATCH] Implement new hook for max_align_t_align
On 10/12/2016 10:17 AM, John David Anglin wrote: > On 2016-10-12 9:48 AM, Jason Merrill wrote: >> On Wed, Oct 12, 2016 at 4:02 AM, Jakub Jelinek wrote: >>> On Wed, Oct 12, 2016 at 09:52:04AM +0200, Florian Weimer wrote: dropping the alignment means that the padding before the lock member vanishes. Consequently, we have just created a silent ABI change in application code, which is a big no-no. >>> Sure, it would be an ABI change, but how many users would it affect? >>> Since this is PA-RISC, which is essentially dead (neither HPE nor Debian ship it anymore), I stand by my suggestion to bump the fundamental alignment >>> Or just drop support for a dead arch? >>> instead. Sure, it is a bit inefficient, but this will only affect PA-RISC users. It does not even cause work for PA-RISC porters. Conversely, if we work on this to come up with a different fix, many more people will be affected (because they don't get all the nice things we could work on instead), and we may need to maintain a special GCC kludge for the alternative solution, impacting GCC developers in particular. >>> But sure, bumping malloc alignment is probably easiest. And people who want >>> performance have better options than to stay on 32-bit PA-RISC anyway. >> Or we could do nothing and tell people to ignore the harmless warning. > The warning is an issue because of -Werror. However, it appears easy to > suppress it in the PA > backend. I have a patch that I'm testing. > > We are discussing offline regarding the glibc issue. It easy to bump the > alignment of malloc > but I take Jakub's point and maybe we should break the ABI. Debian unstable > churns > quickly, and I think we would be better off being consistent with the current > max_align_t > and 8-byte aligned malloc. I am against breaking the ABI. I would rather see us bump malloc alignment up to 16-bytes. The last time I changed this alignment it _immediately_ broken libstdc++ boostrap because it's using exactly the kind of embedded pthread_mutext_t we're talking about breaking. So in that case the debian builds in unstable broke right away, and I had to revert the change. We'd have to BinNMU a bunch of things to get this working again. Again I think our two options are, in my order of preference: - Disable the warning via a PA backend change. - Bump malloc alignment. I am sensitive to the first change being something that carries with it extra maintenance burden, so I'm happy to see the second solution chosen if that's what everyone wants (Florian's suggestion). -- Cheers, Carlos.
Re: [PATCH] Implement new hook for max_align_t_align
On 2016-10-12 2:01 PM, Florian Weimer wrote: On 10/12/2016 06:14 PM, Jeff Law wrote: If the issue is strictly limited to 32 bit hpux, then do we really care? Can we just deprecate that platform and thus make the issue go away? Are we talking about HP-XX or Linux on PA-RISC? The patch was intended to address a Linux issue on PA-RISC. On HP-UX, the current version of max_align_t is fine. Dave -- John David Anglin dave.ang...@bell.net
Re: [PATCH] Implement new hook for max_align_t_align
On 10/12/2016 06:14 PM, Jeff Law wrote: If the issue is strictly limited to 32 bit hpux, then do we really care? Can we just deprecate that platform and thus make the issue go away? Are we talking about HP-XX or Linux on PA-RISC? Thanks, Florian
Re: [PATCH] Implement new hook for max_align_t_align
On 2016-10-12 12:14 PM, Jeff Law wrote: On 10/12/2016 02:02 AM, Jakub Jelinek wrote: On Wed, Oct 12, 2016 at 09:52:04AM +0200, Florian Weimer wrote: dropping the alignment means that the padding before the lock member vanishes. Consequently, we have just created a silent ABI change in application code, which is a big no-no. Sure, it would be an ABI change, but how many users would it affect? Since this is PA-RISC, which is essentially dead (neither HPE nor Debian ship it anymore), I stand by my suggestion to bump the fundamental alignment Or just drop support for a dead arch? instead. Sure, it is a bit inefficient, but this will only affect PA-RISC users. It does not even cause work for PA-RISC porters. Conversely, if we work on this to come up with a different fix, many more people will be affected (because they don't get all the nice things we could work on instead), and we may need to maintain a special GCC kludge for the alternative solution, impacting GCC developers in particular. But sure, bumping malloc alignment is probably easiest. And people who want performance have better options than to stay on 32-bit PA-RISC anyway. If the issue is strictly limited to 32 bit hpux, then do we really care? Can we just deprecate that platform and thus make the issue go away? It's really hard to make an argument to do anything other than deprecate that platform. Given John's ongoing involvement it's much harder to deprecate 64bit linux (and consequently 64bit hpux). I believe that deprecating the 32-bit HP-UX platform makes sense. There is no HP involvement in hppa, ia64 or alpha open source that I am aware of, so deprecating gcc platforms with HP systems is reasonable. My primary focus is open source and one less platform will free some time. We still need 64bit hpux for 64bit linux development. Dave -- John David Anglin dave.ang...@bell.net
Re: [PATCH] Implement new hook for max_align_t_align
On 10/12/2016 02:02 AM, Jakub Jelinek wrote: On Wed, Oct 12, 2016 at 09:52:04AM +0200, Florian Weimer wrote: dropping the alignment means that the padding before the lock member vanishes. Consequently, we have just created a silent ABI change in application code, which is a big no-no. Sure, it would be an ABI change, but how many users would it affect? Since this is PA-RISC, which is essentially dead (neither HPE nor Debian ship it anymore), I stand by my suggestion to bump the fundamental alignment Or just drop support for a dead arch? instead. Sure, it is a bit inefficient, but this will only affect PA-RISC users. It does not even cause work for PA-RISC porters. Conversely, if we work on this to come up with a different fix, many more people will be affected (because they don't get all the nice things we could work on instead), and we may need to maintain a special GCC kludge for the alternative solution, impacting GCC developers in particular. But sure, bumping malloc alignment is probably easiest. And people who want performance have better options than to stay on 32-bit PA-RISC anyway. If the issue is strictly limited to 32 bit hpux, then do we really care? Can we just deprecate that platform and thus make the issue go away? It's really hard to make an argument to do anything other than deprecate that platform. Given John's ongoing involvement it's much harder to deprecate 64bit linux (and consequently 64bit hpux). Jeff
Re: [PATCH] Implement new hook for max_align_t_align
On Wed, 12 Oct 2016, Richard Biener wrote: > I'd say what applies to PA should apply equally well to the pdp11 and > the alpha port ... > > But usually the question is just whether the port has a maintainer > and/or whether it is > a maintainance burden to keep it (say, last user of obsolete feature X). Last users of obsolete feature "defines TARGET_HAVE_NAMED_SECTIONS to false": 32-bit PA HP-UX (64-bit HP-UX is ELF like GNU/Linux, so OK), pdp11, pre-ELF OpenBSD ports. (I'm not aware of that obsolete feature particularly causing problems, however. But the implication for pdp11 would be that if the port were to stay with that feature being removed, it should move to ELF - the e_machine value EM_PDP11 having been allocated by Lars Brinkhoff on 30 May 2002, but I don't know if there's an ABI.) -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] Implement new hook for max_align_t_align
On 2016-10-12 9:48 AM, Jason Merrill wrote: On Wed, Oct 12, 2016 at 4:02 AM, Jakub Jelinek wrote: On Wed, Oct 12, 2016 at 09:52:04AM +0200, Florian Weimer wrote: dropping the alignment means that the padding before the lock member vanishes. Consequently, we have just created a silent ABI change in application code, which is a big no-no. Sure, it would be an ABI change, but how many users would it affect? Since this is PA-RISC, which is essentially dead (neither HPE nor Debian ship it anymore), I stand by my suggestion to bump the fundamental alignment Or just drop support for a dead arch? instead. Sure, it is a bit inefficient, but this will only affect PA-RISC users. It does not even cause work for PA-RISC porters. Conversely, if we work on this to come up with a different fix, many more people will be affected (because they don't get all the nice things we could work on instead), and we may need to maintain a special GCC kludge for the alternative solution, impacting GCC developers in particular. But sure, bumping malloc alignment is probably easiest. And people who want performance have better options than to stay on 32-bit PA-RISC anyway. Or we could do nothing and tell people to ignore the harmless warning. The warning is an issue because of -Werror. However, it appears easy to suppress it in the PA backend. I have a patch that I'm testing. We are discussing offline regarding the glibc issue. It easy to bump the alignment of malloc but I take Jakub's point and maybe we should break the ABI. Debian unstable churns quickly, and I think we would be better off being consistent with the current max_align_t and 8-byte aligned malloc. Dave -- John David Anglin dave.ang...@bell.net
Re: [PATCH] Implement new hook for max_align_t_align
On Wed, Oct 12, 2016 at 4:02 AM, Jakub Jelinek wrote: > On Wed, Oct 12, 2016 at 09:52:04AM +0200, Florian Weimer wrote: >> dropping the alignment means that the padding before the lock member >> vanishes. Consequently, we have just created a silent ABI change in >> application code, which is a big no-no. > > Sure, it would be an ABI change, but how many users would it affect? > >> Since this is PA-RISC, which is essentially dead (neither HPE nor Debian >> ship it anymore), I stand by my suggestion to bump the fundamental alignment > > Or just drop support for a dead arch? > >> instead. Sure, it is a bit inefficient, but this will only affect PA-RISC >> users. It does not even cause work for PA-RISC porters. Conversely, if we >> work on this to come up with a different fix, many more people will be >> affected (because they don't get all the nice things we could work on >> instead), and we may need to maintain a special GCC kludge for the >> alternative solution, impacting GCC developers in particular. > > But sure, bumping malloc alignment is probably easiest. And people who want > performance have better options than to stay on 32-bit PA-RISC anyway. Or we could do nothing and tell people to ignore the harmless warning. Jason
Re: [PATCH] Implement new hook for max_align_t_align
On 10/12/2016 02:43 PM, Richard Biener wrote: I'd say what applies to PA should apply equally well to the pdp11 and the alpha port ... But usually the question is just whether the port has a maintainer and/or whether it is a maintainance burden to keep it (say, last user of obsolete feature X). Well, we seem to be running into a problem with PA, and pdp11 is a cc0 port. Bernd
Re: [PATCH] Implement new hook for max_align_t_align
On Wed, Oct 12, 2016 at 2:33 PM, Bernd Schmidt wrote: > On 10/12/2016 02:12 PM, John David Anglin wrote: >> >> On 2016-10-12, at 4:02 AM, Jakub Jelinek wrote: >> Since this is PA-RISC, which is essentially dead (neither HPE nor Debian ship it anymore), I stand by my suggestion to bump the fundamental alignment >>> >>> >>> Or just drop support for a dead arch? >> >> >> Hardware is still available on the second hand market. > > > So is the Commodore 64, but is that enough though to keep supporting PA in > gcc? Anyone who wants to do retrocomputing can still use gcc-6 or earlier > versions. I'd say what applies to PA should apply equally well to the pdp11 and the alpha port ... But usually the question is just whether the port has a maintainer and/or whether it is a maintainance burden to keep it (say, last user of obsolete feature X). Richard. > > Bernd
Re: [PATCH] Implement new hook for max_align_t_align
On 10/12/2016 02:12 PM, John David Anglin wrote: On 2016-10-12, at 4:02 AM, Jakub Jelinek wrote: Since this is PA-RISC, which is essentially dead (neither HPE nor Debian ship it anymore), I stand by my suggestion to bump the fundamental alignment Or just drop support for a dead arch? Hardware is still available on the second hand market. So is the Commodore 64, but is that enough though to keep supporting PA in gcc? Anyone who wants to do retrocomputing can still use gcc-6 or earlier versions. Bernd
Re: [PATCH] Implement new hook for max_align_t_align
On 2016-10-12, at 4:02 AM, Jakub Jelinek wrote: >> Since this is PA-RISC, which is essentially dead (neither HPE nor Debian >> ship it anymore), I stand by my suggestion to bump the fundamental alignment > > Or just drop support for a dead arch? Hardware is still available on the second hand market. Linux is available from Debian although not as a release architecture: https://buildd.debian.org/status/architecture.php?a=hppa&suite=sid I know from personal use that it is more functional than it has ever been. It is also still available from gentoo as far as I know. Dave -- John David Anglin dave.ang...@bell.net
Re: [PATCH] Implement new hook for max_align_t_align
On Wed, Oct 12, 2016 at 09:52:04AM +0200, Florian Weimer wrote: > dropping the alignment means that the padding before the lock member > vanishes. Consequently, we have just created a silent ABI change in > application code, which is a big no-no. Sure, it would be an ABI change, but how many users would it affect? > Since this is PA-RISC, which is essentially dead (neither HPE nor Debian > ship it anymore), I stand by my suggestion to bump the fundamental alignment Or just drop support for a dead arch? > instead. Sure, it is a bit inefficient, but this will only affect PA-RISC > users. It does not even cause work for PA-RISC porters. Conversely, if we > work on this to come up with a different fix, many more people will be > affected (because they don't get all the nice things we could work on > instead), and we may need to maintain a special GCC kludge for the > alternative solution, impacting GCC developers in particular. But sure, bumping malloc alignment is probably easiest. And people who want performance have better options than to stay on 32-bit PA-RISC anyway. Jakub
Re: [PATCH] Implement new hook for max_align_t_align
On 10/12/2016 09:25 AM, Jakub Jelinek wrote:
No, you can just drop the aligned attributes for HPUX 32-bit, basically
introduce a new ABI. If needed, you could add new symbol versions for
pthread_mutex_* etc. (though, if the current code doesn't care about the
alignment, perhaps you could get away without bumping that).
This is not something which can be solved with symbol versioning. It is
fairly common to embed mutexes into other objects, like this:
struct client {
pthread_mutex_t lock;
struct client *next;
size_t attachment_count;
};
The layout above is fine with the alignment change, but if the
programmer writes this instead:
struct client {
struct client *next;
pthread_mutex_t lock;
size_t attachment_count;
};
dropping the alignment means that the padding before the lock member
vanishes. Consequently, we have just created a silent ABI change in
application code, which is a big no-no.
Since this is PA-RISC, which is essentially dead (neither HPE nor Debian
ship it anymore), I stand by my suggestion to bump the fundamental
alignment instead. Sure, it is a bit inefficient, but this will only
affect PA-RISC users. It does not even cause work for PA-RISC porters.
Conversely, if we work on this to come up with a different fix, many
more people will be affected (because they don't get all the nice things
we could work on instead), and we may need to maintain a special GCC
kludge for the alternative solution, impacting GCC developers in particular.
Thanks,
Florian
Re: [PATCH] Implement new hook for max_align_t_align
On Wed, Oct 12, 2016 at 03:01:51AM -0400, Carlos O'Donell wrote: > On 10/11/2016 04:04 PM, John David Anglin wrote: > > On 2016-10-11 2:50 PM, Jason Merrill wrote: > >> /* Alignment, in bits, a C conformant malloc implementation has to > >> provide. > >> The HP-UX malloc implementation provides a default alignment of 8 > >> bytes. > >> This can be increased with mallopt. The glibc implementation also > >> provides > >> 8-byte alignment. Note that this isn't enough for various POSIX > >> types such > >> as pthread_mutex_t. However, since we no longer need the 16-byte > >> alignment > >> for atomic operations, we ignore the nominal alignment specified > >> for these > >> types. The same is true for long double on 64-bit HP-UX. */ > >> > >> If PA malloc doesn't actually provide 16-byte alignment, this change > >> seems problematic; it will mean any type that wants 16-byte alignment > >> will silently get 8-byte alignment instead. > > > > I agree the situation is something of a mess. On linux, we could bump the > > alignment > > of malloc to 16-bytes. However, Carlos argued that we don't need to and I > > think doing > > so would be detrimental to performance. > > Correct, we do not need a 16-byte alignment at runtime. The problem with cheating is that gcc will then assume the structure is properly aligned and optimize based on that (optimize away alignment checks etc.). > > > The 16-byte alignment was used originally because the ldcw instruction used > > for atomic > > operations in linux threads needs 16-byte alignment. However, the nptl > > pthread > > implementation now uses a kernel helper for atomic operations. It only > > needs > > 4-byte alignment. The largest alignment actually needed is for long double > > (8 bytes). > > However, we can't change the 16-byte alignment without affecting the layout > > of various > > structures. > > Correct, the structure padding needs to continue to be there to match the > original ABI. No, you can just drop the aligned attributes for HPUX 32-bit, basically introduce a new ABI. If needed, you could add new symbol versions for pthread_mutex_* etc. (though, if the current code doesn't care about the alignment, perhaps you could get away without bumping that). > I think the case where a user specifically requests a larger aligment is > still always > bound to fail if they exceed malloc's aligment. So removing the warning just > leaves If users use posix_memalign/memalign/aligned_alloc or the C++17 aligned new, they should be fine. Jakub
Re: [PATCH] Implement new hook for max_align_t_align
On 10/11/2016 04:04 PM, John David Anglin wrote: > On 2016-10-11 2:50 PM, Jason Merrill wrote: >> /* Alignment, in bits, a C conformant malloc implementation has to >> provide. >> The HP-UX malloc implementation provides a default alignment of 8 >> bytes. >> This can be increased with mallopt. The glibc implementation also >> provides >> 8-byte alignment. Note that this isn't enough for various POSIX >> types such >> as pthread_mutex_t. However, since we no longer need the 16-byte >> alignment >> for atomic operations, we ignore the nominal alignment specified >> for these >> types. The same is true for long double on 64-bit HP-UX. */ >> >> If PA malloc doesn't actually provide 16-byte alignment, this change >> seems problematic; it will mean any type that wants 16-byte alignment >> will silently get 8-byte alignment instead. > > I agree the situation is something of a mess. On linux, we could bump the > alignment > of malloc to 16-bytes. However, Carlos argued that we don't need to and I > think doing > so would be detrimental to performance. Correct, we do not need a 16-byte alignment at runtime. > The 16-byte alignment was used originally because the ldcw instruction used > for atomic > operations in linux threads needs 16-byte alignment. However, the nptl > pthread > implementation now uses a kernel helper for atomic operations. It only needs > 4-byte alignment. The largest alignment actually needed is for long double > (8 bytes). > However, we can't change the 16-byte alignment without affecting the layout > of various > structures. Correct, the structure padding needs to continue to be there to match the original ABI. > The same is true for long double on HPUX. Originally, it was planned to > implement it > in hardware and that would have required 16-byte alignment. It was only > implemented > in software with an 8-byte alignment requirement. Somehow, it ended up with > 8 and > 16-byte alignment in the HP 32 and 64-bit compilers, respectively. In both > cases, malloc > has 8-byte alignment. It is possible to increase the "grain" size of HP > malloc to 16 bytes. > > Thus, I don't think the silent reduction to 8-byte alignment matters. > Without the change, > we are faced with spurious warnings from new. I suggested disabling the warnings. https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01445.html Which is what Jason suggests here: https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00786.html Though Florian Weimer suggests just bumping malloc to return 16-byte aligned objects and raising max_align_t, since conceptually that's simple (but will impact performance): https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01446.html I think the case where a user specifically requests a larger aligment is still always bound to fail if they exceed malloc's aligment. So removing the warning just leaves hppa where it is today. No warning. Working with existing malloc alignment. But unable to warn users of legitimate cases where this might matter? I still suggest disabling the warning. -- Cheers, Carlos.
Re: [PATCH] Implement new hook for max_align_t_align
On Tue, Oct 11, 2016 at 4:54 PM, John David Anglin wrote: > On 2016-10-11 4:11 PM, Jason Merrill wrote: >> >> On Tue, Oct 11, 2016 at 2:59 PM, DJ Delorie wrote: >>> >>> Jason Merrill writes: If PA malloc doesn't actually provide 16-byte alignment, this change seems problematic; it will mean any type that wants 16-byte alignment will silently get 8-byte alignment instead. >>> >>> Should such cases be calling memalign (or posix_memalign) instead of >>> malloc? >> >> We're talking about this in the context of C++17 aligned new, which >> uses one of those functions (or C aligned_alloc) under the hood. >> Currently on PA, allocating one of these types with 'new' in C++14 >> mode gives a warning because the compiler doesn't think the allocation >> will actually provide the 16-byte alignment that the type wants. This >> warning seems to be correct. This patch would silence that warning by >> pretending that malloc will provide 16-byte alignment, which is not >> actually true. > > But the check isn't directly about malloc. It's between the alignment for > max_align_t and the alignment that the type wants. Joseph indicated that > max_align_t > should have an alignment as large as the POSIX types (e.g., pthread_mutex_t). > So, I think > setting it to 16 when pthread_mutex_t wants an alignment of 16 is correct. The connection to max_align_t is just a GCC implementation detail; the C++ standard doesn't tie use of aligned new to alignof(max_align_t), there's a separate macro __STDCPP_DEFAULT_NEW_ALIGNMENT__. This was done specifically to allow for the case where malloc for a target provides more alignment than max_align_t. But that still doesn't help with this case, where in fact the warning is correct, you just know it isn't a problem. So disabling the warning on PA seems like the way to go. Jason
Re: [PATCH] Implement new hook for max_align_t_align
On Tue, Oct 11, 2016 at 04:54:56PM -0400, John David Anglin wrote: > But the check isn't directly about malloc. It's between the alignment for > max_align_t > and the alignment that the type wants. Joseph indicated that max_align_t > should have an > alignment as large as the POSIX types (e.g., pthread_mutex_t). So, I think > setting it to 16 > when pthread_mutex_t wants an alignment of 16 is correct. But precondition of bumping max_align_t to 16 is IMHO that malloc actually provides that alignment. If that is not the case, then I think you should just drop the alignment attribute from pthread_mutex_t etc. and deal with the ABI change. Jakub
Re: [PATCH] Implement new hook for max_align_t_align
On 2016-10-11 4:11 PM, Jason Merrill wrote: On Tue, Oct 11, 2016 at 2:59 PM, DJ Delorie wrote: Jason Merrill writes: If PA malloc doesn't actually provide 16-byte alignment, this change seems problematic; it will mean any type that wants 16-byte alignment will silently get 8-byte alignment instead. Should such cases be calling memalign (or posix_memalign) instead of malloc? We're talking about this in the context of C++17 aligned new, which uses one of those functions (or C aligned_alloc) under the hood. Currently on PA, allocating one of these types with 'new' in C++14 mode gives a warning because the compiler doesn't think the allocation will actually provide the 16-byte alignment that the type wants. This warning seems to be correct. This patch would silence that warning by pretending that malloc will provide 16-byte alignment, which is not actually true. But the check isn't directly about malloc. It's between the alignment for max_align_t and the alignment that the type wants. Joseph indicated that max_align_t should have an alignment as large as the POSIX types (e.g., pthread_mutex_t). So, I think setting it to 16 when pthread_mutex_t wants an alignment of 16 is correct. -- John David Anglin dave.ang...@bell.net
Re: [PATCH] Implement new hook for max_align_t_align
On Tue, Oct 11, 2016 at 2:59 PM, DJ Delorie wrote: > > Jason Merrill writes: >> If PA malloc doesn't actually provide 16-byte alignment, this change >> seems problematic; it will mean any type that wants 16-byte alignment >> will silently get 8-byte alignment instead. > > Should such cases be calling memalign (or posix_memalign) instead of > malloc? We're talking about this in the context of C++17 aligned new, which uses one of those functions (or C aligned_alloc) under the hood. Currently on PA, allocating one of these types with 'new' in C++14 mode gives a warning because the compiler doesn't think the allocation will actually provide the 16-byte alignment that the type wants. This warning seems to be correct. This patch would silence that warning by pretending that malloc will provide 16-byte alignment, which is not actually true. It seems to me that the warning is correct, but not a problem in this case, so perhaps turning the warning off by default on PA is the right solution. Jason
Re: [PATCH] Implement new hook for max_align_t_align
On 2016-10-11 2:50 PM, Jason Merrill wrote: /* Alignment, in bits, a C conformant malloc implementation has to provide. The HP-UX malloc implementation provides a default alignment of 8 bytes. This can be increased with mallopt. The glibc implementation also provides 8-byte alignment. Note that this isn't enough for various POSIX types such as pthread_mutex_t. However, since we no longer need the 16-byte alignment for atomic operations, we ignore the nominal alignment specified for these types. The same is true for long double on 64-bit HP-UX. */ If PA malloc doesn't actually provide 16-byte alignment, this change seems problematic; it will mean any type that wants 16-byte alignment will silently get 8-byte alignment instead. I agree the situation is something of a mess. On linux, we could bump the alignment of malloc to 16-bytes. However, Carlos argued that we don't need to and I think doing so would be detrimental to performance. The 16-byte alignment was used originally because the ldcw instruction used for atomic operations in linux threads needs 16-byte alignment. However, the nptl pthread implementation now uses a kernel helper for atomic operations. It only needs 4-byte alignment. The largest alignment actually needed is for long double (8 bytes). However, we can't change the 16-byte alignment without affecting the layout of various structures. The same is true for long double on HPUX. Originally, it was planned to implement it in hardware and that would have required 16-byte alignment. It was only implemented in software with an 8-byte alignment requirement. Somehow, it ended up with 8 and 16-byte alignment in the HP 32 and 64-bit compilers, respectively. In both cases, malloc has 8-byte alignment. It is possible to increase the "grain" size of HP malloc to 16 bytes. Thus, I don't think the silent reduction to 8-byte alignment matters. Without the change, we are faced with spurious warnings from new. -- John David Anglin dave.ang...@bell.net
Re: [PATCH] Implement new hook for max_align_t_align
Jason Merrill writes: > If PA malloc doesn't actually provide 16-byte alignment, this change > seems problematic; it will mean any type that wants 16-byte alignment > will silently get 8-byte alignment instead. Should such cases be calling memalign (or posix_memalign) instead of malloc?
Re: [PATCH] Implement new hook for max_align_t_align
/* Alignment, in bits, a C conformant malloc implementation has to provide. The HP-UX malloc implementation provides a default alignment of 8 bytes. This can be increased with mallopt. The glibc implementation also provides 8-byte alignment. Note that this isn't enough for various POSIX types such as pthread_mutex_t. However, since we no longer need the 16-byte alignment for atomic operations, we ignore the nominal alignment specified for these types. The same is true for long double on 64-bit HP-UX. */ If PA malloc doesn't actually provide 16-byte alignment, this change seems problematic; it will mean any type that wants 16-byte alignment will silently get 8-byte alignment instead. Jason
Re: [PATCH] Implement new hook for max_align_t_align
Attached is an updated version using the new builtin __MAX_ALIGN_T_ALIGN__.
This
simplifies the declaration of max_align_t and ensures it is always the same as
max_align_t_align().
Tested on hppa-unknown-linux-gnu. Okay for trunk?
Dave
--
John David Anglin dave.ang...@bell.net
2016-10-10 John David Anglin
gcc/c-family/
* c-common.c (c_stddef_cpp_builtins): Add __MAX_ALIGN_T_ALIGN__ builtin
define.
(max_align_t_align): Move to targhooks.c.
* c-common.h (max_align_t_align): Delete.
gcc/
* target.def (max_align_t_align): New target hook.
* targhooks.c (default_max_align_t_align): New.
* targhooks.h (default_max_align_t_align): Declare.
* config/pa/pa.c (pa_max_align_t_align): New.
(TARGET_MAX_ALIGN_T_ALIGN): Define.
* ginclude/stddef.h (max_align_t): Use __MAX_ALIGN_T_ALIGN__ builtin.
* doc/tm.texi.in (TARGET_MAX_ALIGN_T_ALIGN): Add documentation hook.
* doc/tm.texi: Update.
gcc/cp/
* decl.c (cxx_init_decl_processing): Use max_align_t_align target hook.
* init.c (build_new_1): Likewise.
Index: c-family/c-common.c
===
--- c-family/c-common.c (revision 240901)
+++ c-family/c-common.c (working copy)
@@ -6683,6 +6683,8 @@
builtin_define_with_value ("__INTPTR_TYPE__", INTPTR_TYPE, 0);
if (UINTPTR_TYPE)
builtin_define_with_value ("__UINTPTR_TYPE__", UINTPTR_TYPE, 0);
+ builtin_define_with_int_value ("__MAX_ALIGN_T_ALIGN__",
+targetm.max_align_t_align () / BITS_PER_UNIT);
}
static void
@@ -12925,22 +12927,6 @@
return stv_nothing;
}
-/* Return the alignment of std::max_align_t.
-
- [support.types.layout] The type max_align_t is a POD type whose alignment
- requirement is at least as great as that of every scalar type, and whose
- alignment requirement is supported in every context. */
-
-unsigned
-max_align_t_align ()
-{
- unsigned int max_align = MAX (TYPE_ALIGN (long_long_integer_type_node),
- TYPE_ALIGN (long_double_type_node));
- if (float128_type_node != NULL_TREE)
-max_align = MAX (max_align, TYPE_ALIGN (float128_type_node));
- return max_align;
-}
-
/* Return true iff ALIGN is an integral constant that is a fundamental
alignment, as defined by [basic.align] in the c++-11
specifications.
@@ -12954,7 +12940,7 @@
bool
cxx_fundamental_alignment_p (unsigned align)
{
- return (align <= max_align_t_align ());
+ return (align <= targetm.max_align_t_align ());
}
/* Return true if T is a pointer to a zero-sized aggregate. */
Index: c-family/c-common.h
===
--- c-family/c-common.h (revision 240901)
+++ c-family/c-common.h (working copy)
@@ -866,7 +866,6 @@
extern bool keyword_is_storage_class_specifier (enum rid);
extern bool keyword_is_type_qualifier (enum rid);
extern bool keyword_is_decl_specifier (enum rid);
-extern unsigned max_align_t_align (void);
extern bool cxx_fundamental_alignment_p (unsigned);
extern bool pointer_to_zero_sized_aggr_p (tree);
extern bool diagnose_mismatched_attributes (tree, tree);
Index: config/pa/pa.c
===
--- config/pa/pa.c (revision 240901)
+++ config/pa/pa.c (working copy)
@@ -194,6 +194,7 @@
static bool pa_legitimate_constant_p (machine_mode, rtx);
static unsigned int pa_section_type_flags (tree, const char *, int);
static bool pa_legitimate_address_p (machine_mode, rtx, bool);
+static unsigned int pa_max_align_t_align (void);
/* The following extra sections are only used for SOM. */
static GTY(()) section *som_readonly_data_section;
@@ -400,6 +401,9 @@
#undef TARGET_LRA_P
#define TARGET_LRA_P hook_bool_void_false
+#undef TARGET_MAX_ALIGN_T_ALIGN
+#define TARGET_MAX_ALIGN_T_ALIGN pa_max_align_t_align
+
struct gcc_target targetm = TARGET_INITIALIZER;
/* Parse the -mfixed-range= option string. */
@@ -10719,4 +10723,16 @@
return NULL_RTX;
}
+/* The maximimum alignment in bits for the POD type std:max_align_t.
+ This is set to 128 on 32-bit non HP-UX systems to suppress warnings
+ about new with extended alignment. This arises because various POSIX
+ types such as pthread_mutex_t have for historical reasons 128-bit
+ alignment but the default alignment of std:max_align_t is 64 bits. */
+
+static unsigned int
+pa_max_align_t_align (void)
+{
+ return TARGET_HPUX && !TARGET_64BIT ? 64 : 128;
+}
+
#include "gt-pa.h"
Index: cp/decl.c
===
--- cp/decl.c (revision 240901)
+++ cp/decl.c (working copy)
@@ -4082,7 +4082,7 @@
if (aligned_new_threshold == -1)
aligned_new_threshold = (cxx_dialect >= cxx1z) ? 1 : 0;
if (aligned_new_threshold == 1)
-aligned_new_threshold = max_align_t_align () / BITS_PER_UNIT;
+aligned_new_threshold = targ
Re: [PATCH] Implement new hook for max_align_t_align
On 2016-10-09, at 4:34 AM, Bernd Edlinger wrote:
> For instance have:
>
> typedef struct {
> char __max_align[0] __attribute__((__aligned__(__MAX_ALIGN_T_ALIGN__)));
> } max_align_t;
>
> Provided we do:
>
> builtin_define_with_value ("__MAX_ALIGN_T_ALIGN__",
> targetm.max_align_t_align () / BITS_PER_UNIT);
>
> Would'nt that guarantee, that __max_align_t and
> max_align_t_align () will always be the same?
Yes, excellent suggestion. Testing.
--
John David Anglin dave.ang...@bell.net
Re: [PATCH] Implement new hook for max_align_t_align
On 10/08/16 19:36, John David Anglin wrote:
> On 2016-10-08, at 1:01 PM, Bernd Edlinger wrote:
>
>> I think your callback should also directly control the
>> alignment of max_align_t in stddef.h:
>>
>>
>>long long __max_align_ll __attribute__((__aligned__(__alignof__(long
>> long;
>>long double __max_align_ld
>> __attribute__((__aligned__(__alignof__(long double;
>>/* _Float128 is defined as a basic type, so max_align_t must be
>> sufficiently aligned for it. This code must work in C++, so we
>> use __float128 here; that is only available on some
>> architectures, but only on i386 is extra alignment needed for
>> __float128. */
>> #ifdef __i386__
>>__float128 __max_align_f128
>> __attribute__((__aligned__(__alignof(__float128;
>> #endif
>> } max_align_t;
>>
>>
>> otherwise these will not match.
>
> Yes, i missed a hunk in the submission. On hpux, the alignment is determined
> by the long
> double field. With glibc, we need 16 byte alignment for max_align_t.
>
This looks still brittle to me.
I mean also the defines __hppa__ and __hpux__ come from
builtin_define calls in the backend, why not define
something with builtin_define_with_int_value,
which can be used as is in max_align_t.
For instance have:
typedef struct {
char __max_align[0] __attribute__((__aligned__(__MAX_ALIGN_T_ALIGN__)));
} max_align_t;
Provided we do:
builtin_define_with_value ("__MAX_ALIGN_T_ALIGN__",
targetm.max_align_t_align () / BITS_PER_UNIT);
Would'nt that guarantee, that __max_align_t and
max_align_t_align () will always be the same?
Bernd.
Re: [PATCH] Implement new hook for max_align_t_align
On 2016-10-08, at 1:01 PM, Bernd Edlinger wrote:
> I think your callback should also directly control the
> alignment of max_align_t in stddef.h:
>
> typedef struct {
> long long __max_align_ll __attribute__((__aligned__(__alignof__(long
> long;
> long double __max_align_ld
> __attribute__((__aligned__(__alignof__(long double;
> /* _Float128 is defined as a basic type, so max_align_t must be
> sufficiently aligned for it. This code must work in C++, so we
> use __float128 here; that is only available on some
> architectures, but only on i386 is extra alignment needed for
> __float128. */
> #ifdef __i386__
> __float128 __max_align_f128
> __attribute__((__aligned__(__alignof(__float128;
> #endif
> } max_align_t;
>
>
> otherwise these will not match.
Yes, i missed a hunk in the submission. On hpux, the alignment is determined
by the long
double field. With glibc, we need 16 byte alignment for max_align_t.
Dave
--
John David Anglin dave.ang...@bell.net
2016-10-08 John David Anglin
gcc/c-family/
* c-common.c (max_align_t_align): Move to targhooks.c.
* c-common.h (max_align_t_align): Delete.
gcc/
* target.def (max_align_t_align): New target hook.
* targhooks.c (default_max_align_t_align): New.
* targhooks.h (default_max_align_t_align): Declare.
* config/pa/pa.h (BIGGEST_ALIGNMENT): Adjust comment.
(MALLOC_ABI_ALIGNMENT): Define.
* config/pa/pa.c (pa_max_align_t_align): New.
(TARGET_MAX_ALIGN_T_ALIGN): Define.
* ginclude/stddef.h (max_align_t): Align to 16 bytes on non-hpux hppa.
* doc/tm.texi.in (TARGET_MAX_ALIGN_T_ALIGN): Add documentation hook.
* doc/tm.texi: Update.
gcc/cp/
* decl.c (cxx_init_decl_processing): Use max_align_t_align target hook.
* init.c (build_new_1): Likewise.
Index: c-family/c-common.c
===
--- c-family/c-common.c (revision 240688)
+++ c-family/c-common.c (working copy)
@@ -12899,22 +12899,6 @@
return stv_nothing;
}
-/* Return the alignment of std::max_align_t.
-
- [support.types.layout] The type max_align_t is a POD type whose alignment
- requirement is at least as great as that of every scalar type, and whose
- alignment requirement is supported in every context. */
-
-unsigned
-max_align_t_align ()
-{
- unsigned int max_align = MAX (TYPE_ALIGN (long_long_integer_type_node),
- TYPE_ALIGN (long_double_type_node));
- if (float128_type_node != NULL_TREE)
-max_align = MAX (max_align, TYPE_ALIGN (float128_type_node));
- return max_align;
-}
-
/* Return true iff ALIGN is an integral constant that is a fundamental
alignment, as defined by [basic.align] in the c++-11
specifications.
@@ -12928,7 +12912,7 @@
bool
cxx_fundamental_alignment_p (unsigned align)
{
- return (align <= max_align_t_align ());
+ return (align <= targetm.max_align_t_align ());
}
/* Return true if T is a pointer to a zero-sized aggregate. */
Index: c-family/c-common.h
===
--- c-family/c-common.h (revision 240688)
+++ c-family/c-common.h (working copy)
@@ -864,7 +864,6 @@
extern bool keyword_is_storage_class_specifier (enum rid);
extern bool keyword_is_type_qualifier (enum rid);
extern bool keyword_is_decl_specifier (enum rid);
-extern unsigned max_align_t_align (void);
extern bool cxx_fundamental_alignment_p (unsigned);
extern bool pointer_to_zero_sized_aggr_p (tree);
extern bool diagnose_mismatched_attributes (tree, tree);
Index: config/pa/pa.c
===
--- config/pa/pa.c (revision 240688)
+++ config/pa/pa.c (working copy)
@@ -194,6 +194,7 @@
static bool pa_legitimate_constant_p (machine_mode, rtx);
static unsigned int pa_section_type_flags (tree, const char *, int);
static bool pa_legitimate_address_p (machine_mode, rtx, bool);
+static unsigned int pa_max_align_t_align (void);
/* The following extra sections are only used for SOM. */
static GTY(()) section *som_readonly_data_section;
@@ -400,6 +401,9 @@
#undef TARGET_LRA_P
#define TARGET_LRA_P hook_bool_void_false
+#undef TARGET_MAX_ALIGN_T_ALIGN
+#define TARGET_MAX_ALIGN_T_ALIGN pa_max_align_t_align
+
struct gcc_target targetm = TARGET_INITIALIZER;
/* Parse the -mfixed-range= option string. */
@@ -10719,4 +10723,16 @@
return NULL_RTX;
}
+/* The maximimum alignment in bits for the POD type std:max_align_t.
+ This is set to 128 on 32-bit non HP-UX systems to suppress warnings
+ about new with extended alignment. This arises because various POSIX
+ types such as pthread_mutex_t have for historical reasons 128-bit
+ alignment but the default alignment of std:max_align_t is 64 bits. */
+
+static unsigned int
+pa_max_align_t_align (void)
+{
+ return TARGET_HPUX && !TARGET_64BIT ? 64 : 128;
+}
+
#include
Re: [PATCH] Implement new hook for max_align_t_align
bounced, resent...
Hi,
I think your callback should also directly control the
alignment of max_align_t in stddef.h:
typedef struct {
long long __max_align_ll __attribute__((__aligned__(__alignof__(long
long;
long double __max_align_ld
__attribute__((__aligned__(__alignof__(long double;
/* _Float128 is defined as a basic type, so max_align_t must be
sufficiently aligned for it. This code must work in C++, so we
use __float128 here; that is only available on some
architectures, but only on i386 is extra alignment needed for
__float128. */
#ifdef __i386__
__float128 __max_align_f128
__attribute__((__aligned__(__alignof(__float128;
#endif
} max_align_t;
otherwise these will not match.
I think is __alignof(max_aling_t) == max_align_t_align ()
on your target?
Bernd.
Re: [PATCH] Implement new hook for max_align_t_align
Hi,
I think your callback should also directly control the
alignment of max_align_t in stddef.h:
typedef struct {
long long __max_align_ll __attribute__((__aligned__(__alignof__(long
long;
long double __max_align_ld
__attribute__((__aligned__(__alignof__(long double;
/* _Float128 is defined as a basic type, so max_align_t must be
sufficiently aligned for it. This code must work in C++, so we
use __float128 here; that is only available on some
architectures, but only on i386 is extra alignment needed for
__float128. */
#ifdef __i386__
__float128 __max_align_f128
__attribute__((__aligned__(__alignof(__float128;
#endif
} max_align_t;
otherwise these will not match.
Bernd.
