Bug 57350 - std::align missing
Summary: std::align missing
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 4.8.0
: P3 normal
Target Milestone: 5.0
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-21 08:47 UTC by David Krauss
Modified: 2014-10-13 14:11 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2014-02-04 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Krauss 2013-05-21 08:47:27 UTC
C++11 §20.6.5 [ptr.align] remains unimplemented.

Several years ago I published what now appears to be a compliant implementation, but it was under the MIT license. Does that disqualify that code, or me, from submitting a patch?

As pure, relatively agnostic pointer arithmetic it doesn't seem to fit into the other groups of functions specified for <memory>, which are implemented in various separate files.
Comment 1 David Krauss 2013-05-21 08:48:09 UTC
Oh, here's a link to my version: http://code.google.com/p/c-plus/source/browse/src/util.h#50
Comment 2 Jonathan Wakely 2013-05-21 09:58:42 UTC
(In reply to David Krauss from comment #0)
> C++11 §20.6.5 [ptr.align] remains unimplemented.
> 
> Several years ago I published what now appears to be a compliant
> implementation, but it was under the MIT license. Does that disqualify that
> code, or me, from submitting a patch?

Not at all, nothing stops you (as the author) submitting it to the FSF under the GPL, but to accept it the FSF usually need a copyright asignment on file - do you have one?
Comment 3 Vladimir Krivopalov 2014-02-01 17:06:04 UTC
Greetings,

Any chance to get std::align() implementation included in the coming GCC releases?
What needs to be done for that?
Comment 4 David Krauss 2014-02-02 02:58:32 UTC
Hmm, I recall preparing to submit a patch but not being able to decide which header to modify.

Here's the aforementioned MIT-licensed code. The MIT license only requires attribution which is satisfied by the changelog; anyway I don't care. I am already a GNU contributor with FSF waiver back in 2009.

This code should probably be reviewed. I wrote it a long time ago and seldom used it. Cannot recall whether it was intended to be 100% compliant.


inline void *align( std::size_t alignment, std::size_t size, void *&ptr, std::size_t &space ) {
	auto pn = reinterpret_cast< std::size_t >( ptr );
	auto aligned = ( pn + alignment - 1 ) & - alignment;
	auto new_space = space - ( aligned - pn );
	if ( new_space < size ) return nullptr;
	space = new_space;
	return ptr = reinterpret_cast< void * >( aligned );
}
Comment 5 David Krauss 2014-02-02 03:08:38 UTC
Just re-reading now, std::size_t should be std::uintptr_t, but I don't see anything else that could cause UB. The bitwise "negative" arithmetic should be OK because it's all on unsigned values.

And if GNU style doesn't allow auto, those should just be uintptr_t or size_t as appropriate.
Comment 6 Vladimir Krivopalov 2014-02-02 04:14:23 UTC
(In reply to David Krauss from comment #5)
> Just re-reading now, std::size_t should be std::uintptr_t, but I don't see
> anything else that could cause UB. The bitwise "negative" arithmetic should
> be OK because it's all on unsigned values.
> 
> And if GNU style doesn't allow auto, those should just be uintptr_t or
> size_t as appropriate.

This code looks fine to me at my best knowledge of expected std::align() behaviour.

I also tried it against the artificial test case described at https://stackoverflow.com/questions/16305311/usage-issue-of-stdalign and it doesn't re-align the already aligned pointer.

Not sure if auto keyword is prohibited by GCC internal code style, perhaps someone from GCC devs could help on that.

Thank you for preparing the fix, David!
Comment 7 David Krauss 2014-02-02 04:47:39 UTC
Haha, it looks like the MSVC devs forgot to subtract 1. Typical.

I did test my code in a real arena allocator, by the way, so that sort of thing would not have gotten through.
Comment 8 Jonathan Wakely 2014-02-03 07:16:28 UTC
(In reply to Vladimir Krivopalov from comment #3)
> Any chance to get std::align() implementation included in the coming GCC
> releases?
> What needs to be done for that?

It's too late for GCC 4.9 now.
Comment 9 David Krauss 2014-02-04 07:03:32 UTC
Whoa, there's a nasty bug there, if alignment > space.

inline void *align( std::size_t alignment, std::size_t size,
                    void *&ptr, std::size_t &space ) {
	std::uintptr_t pn = reinterpret_cast< std::uintptr_t >( ptr );
	std::uintptr_t aligned = ( pn + alignment - 1 ) & - alignment;
	size += aligned - pn; // Add padding to size.
	if ( space < size ) return nullptr;
	space -= size;
	return ptr = reinterpret_cast< void * >( aligned );
}

I haven't tested this edit at all.
Comment 10 Jonathan Wakely 2014-02-04 08:29:44 UTC
Confirmed as unimplemented.
Comment 11 David Krauss 2014-02-05 04:21:24 UTC
No, that code wasn't right either. I'll just leave it at this and a caveat to the reader:

inline void *align( std::size_t alignment, std::size_t size,
                    void *&ptr, std::size_t &space ) {
	std::uintptr_t pn = reinterpret_cast< std::uintptr_t >( ptr );
	std::uintptr_t aligned = ( pn + alignment - 1 ) & - alignment;
	std::size_t padding = aligned - pn;
	if ( space < size + padding ) return nullptr;
	space -= padding;
	return ptr = reinterpret_cast< void * >( aligned );
}
Comment 12 Jonathan Wakely 2014-10-13 14:09:16 UTC
Author: redi
Date: Mon Oct 13 14:08:44 2014
New Revision: 216149

URL: https://gcc.gnu.org/viewcvs?rev=216149&root=gcc&view=rev
Log:
	PR libstdc++/57350
	* include/std/memory (align): Do not adjust correctly aligned address.
	* testsuite/20_util/align/2.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/20_util/align/2.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/std/memory
Comment 13 Jonathan Wakely 2014-10-13 14:11:33 UTC
Fixed on trunk.