Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10796020:
Upgrade AlignedMemory to support dynamic allocations. (Closed)
|
Created:
8 years, 5 months ago by DaleCurtis Modified:
8 years, 5 months ago Reviewers:
willchan no longer on Chromium, jvoung (off chromium), jvoung - send to chromium..., jbates, Jeffrey Yasskin, sky CC:
chromium-reviews, scherkus (not reviewing), jvoung (off chromium), dmichael (off chromium) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpgrade AlignedMemory to support dynamic allocations.
Adds two new methods: AlignedAlloc and AlignedFree for creating and
destroying dynamic aligned allocations respectively. Also adds a
helper class, ScopedPtrAlignedFree, for use with scoped_ptr_malloc.
AlignedAlloc uses posix_memalign for OS X (now that we're targeting
10.6), Linux and _aligned_alloc() on Windows. Android and NaCl use
memalign() since they do not expose posix_memalign() and memalign()
is safe to use with free() on those platforms.
Also hacks around a bug in Visual C++ where __alignof will sometimes
return zero:
http://connect.microsoft.com/VisualStudio/feedback/details/682695/c-alignof-fails-to-properly-evalute-alignment-of-dependent-types
BUG=none
TEST=base_unittests + new test, trybots.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147988
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148483
Patch Set 1 : Documentation. #
Total comments: 18
Patch Set 2 : Comments. Remove unused imports. #
Total comments: 30
Patch Set 3 : Remove templates. #
Total comments: 16
Patch Set 4 : Comments. #Patch Set 5 : Fix headers. #
Total comments: 14
Patch Set 6 : More comments! #
Total comments: 6
Patch Set 7 : Final Fixes! #
Total comments: 1
Patch Set 8 : Use shortened URL to keep CQ happy. #Patch Set 9 : Change AlignedMemory back to POD. Retry now that NaCl fixed. #
Messages
Total messages: 39 (0 generated)
jbates: PTAL. This is different than what we discussed because overriding the new/delete operators in the original AlignedMemory class doesn't work for run time sized use cases (the template class needs an explicit size specialization). scherkus: cc. Let me know if I misunderstood what you were thinking. It was a chore getting this working on all the bots as I ran into a Visual C++ bug with __alignof() in the process that had me stumped for hours. I'll bring in an OWNER for review once we've hashed everything out. Please suggest reviewers as well. I noticed your original CL generated a lot of discussion.
LGTM considering the challenges and constraints. I like that this also works as a member of a dynamically allocated class without risk of losing alignment. A few nits below. https://chromiumcodereview.appspot.com/10796020/diff/28002/base/compiler_spec... File base/compiler_specific.h (right): https://chromiumcodereview.appspot.com/10796020/diff/28002/base/compiler_spec... base/compiler_specific.h:123: #define ALIGNOF(type) (sizeof(type) - sizeof(type)) + __alignof(type) parens should probably go around the whole expression. https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... File base/memory/aligned_memory.h (right): https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:49: ptr = _aligned_malloc(size, align); typically don't put the semicolon here https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:51: _aligned_free(ptr); no semicolon? https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:55: ptr = memalign(align, size); no semicolon? https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:61: #define INTERNAL_ALIGNED_MALLOC(ptr, align, size) \ just to be pedantic in case someone uses this in an if statement, how about a do/while(0) here: do { \ ... } while(0) https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:66: free(ptr); no semicolon? https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:100: DCHECK_EQ(byte_alignment & (byte_alignment - 1), 0); \ is this necessary here since all the alignments are declared below? (dcheck is pretty heavy weight, so just a compile time/ code bloat concern) https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:101: DCHECK_EQ(byte_alignment % sizeof(void*), static_cast<size_t>(0)); \ would it make sense to just do a MAX(byte_alignment, sizeof(void*)) in places where it's used? seems less awkward since the 1, 2, and 4 byte alignments are declared for this template.
drive-by fyi but otherwise looking fine! https://chromiumcodereview.appspot.com/10796020/diff/28002/base/compiler_spec... File base/compiler_specific.h (right): https://chromiumcodereview.appspot.com/10796020/diff/28002/base/compiler_spec... base/compiler_specific.h:119: // http://goo.gl/isH0C FYI it's ok to exceed 80 chars for long urls
+willchan: You reviewed the original AlignedMemory CL please OWNERS review base/ changes here. +sky: Please OWNERS review base/logging.h header additions in chrome/[browser|common] that were necessary since logging.h isn't automatically included with AlignedMemory anymore. Thanks in advance! https://chromiumcodereview.appspot.com/10796020/diff/28002/base/compiler_spec... File base/compiler_specific.h (right): https://chromiumcodereview.appspot.com/10796020/diff/28002/base/compiler_spec... base/compiler_specific.h:119: // http://goo.gl/isH0C On 2012/07/19 21:49:39, scherkus wrote: > FYI it's ok to exceed 80 chars for long urls Yeah, this one is 131 chars though, so it seemed nicer to shorten it. https://chromiumcodereview.appspot.com/10796020/diff/28002/base/compiler_spec... base/compiler_specific.h:123: #define ALIGNOF(type) (sizeof(type) - sizeof(type)) + __alignof(type) On 2012/07/19 03:45:37, jbates wrote: > parens should probably go around the whole expression. Done. https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... File base/memory/aligned_memory.h (right): https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:49: ptr = _aligned_malloc(size, align); On 2012/07/19 03:45:37, jbates wrote: > typically don't put the semicolon here Done. https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:51: _aligned_free(ptr); On 2012/07/19 03:45:37, jbates wrote: > no semicolon? Done. https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:55: ptr = memalign(align, size); On 2012/07/19 03:45:37, jbates wrote: > no semicolon? Done. https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:61: #define INTERNAL_ALIGNED_MALLOC(ptr, align, size) \ On 2012/07/19 03:45:37, jbates wrote: > just to be pedantic in case someone uses this in an if statement, how about a > do/while(0) here: > do { \ > ... > } while(0) Done. https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:66: free(ptr); On 2012/07/19 03:45:37, jbates wrote: > no semicolon? Done. https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:100: DCHECK_EQ(byte_alignment & (byte_alignment - 1), 0); \ On 2012/07/19 03:45:37, jbates wrote: > is this necessary here since all the alignments are declared below? (dcheck is > pretty heavy weight, so just a compile time/ code bloat concern) Good point. Removed. https://chromiumcodereview.appspot.com/10796020/diff/28002/base/memory/aligne... base/memory/aligned_memory.h:101: DCHECK_EQ(byte_alignment % sizeof(void*), static_cast<size_t>(0)); \ On 2012/07/19 03:45:37, jbates wrote: > would it make sense to just do a MAX(byte_alignment, sizeof(void*)) in places > where it's used? seems less awkward since the 1, 2, and 4 byte alignments are > declared for this template. I've removed all of this in favor of forcing a NULL dereference on a failed allocation and selecting which templates to specialize based on 64-bit or 32-bit.
+jyasskin, can you do a first pass here for me? Thanks.
LGTM
https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... File base/memory/aligned_memory.h (right): https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... base/memory/aligned_memory.h:35: #define _XOPEN_SOURCE 600 // for posix_memalign If _XOPEN_SOURCE was already defined as <600, this will cause a duplicate definition warning/error. https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... base/memory/aligned_memory.h:55: ptr = memalign(align, size) http://linux.die.net/man/3/memalign says, "Some systems provide no way to reclaim memory allocated with memalign() or valloc() (because one can only pass to free(3) a pointer gotten from malloc(3), while, for example, memalign() would call malloc(3) and then align the obtained value)." Could you link to the android documentation saying this is safe? https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine What error do they give? It's very weird that <stdlib.h> wouldn't be enough and may indicate something else going on. https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... base/memory/aligned_memory.h:98: // Run time based aligned memory allocator. Since the alignment allocators may Why do you want the posix_memalign wrapper to be named the same as the non-allocating class? It seems better to use different names. I suspect the memalign wrapper should be a function returning a scoped_ptr_malloc<T, AlignedFree> instead of a class. https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... base/memory/aligned_memory.h:109: if (!data_) *reinterpret_cast<uint8*>(data_) = 0; \ Assigning to a null pointer is undefined behavior and may be optimized away instead of causing a crash. Use CHECK instead.
http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:35: #define _XOPEN_SOURCE 600 // for posix_memalign On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > If _XOPEN_SOURCE was already defined as <600, this will cause a duplicate > definition warning/error. Well if we keep the extern below this isn't really necessary anyways, so I can remove it. Alternatively, do you have a suggested improvement? http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:55: ptr = memalign(align, size) On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > http://linux.die.net/man/3/memalign says, "Some systems provide no way to > reclaim memory allocated with memalign() or valloc() (because one can only pass > to free(3) a pointer gotten from malloc(3), while, for example, memalign() would > call malloc(3) and then align the obtained value)." Could you link to the > android documentation saying this is safe? Sadly, NDK documentation is incredibly sparse. I tried this on a hunch to get the android trybot to pass, but have no documentation to back it up and that bot doesn't run the unit tests (or valgrind for that matter). Do we have any Android experts in the house that could answer? I downloaded the NDK and grep'd around without finding anything useful. (no online source search makes me sad :( I'm happy turning this into a NOTREACHED() or something similar for now. http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > What error do they give? It's very weird that <stdlib.h> wouldn't be enough and > may indicate something else going on. "error: ‘posix_memalign’ was not declared in this scope" -- same message on all posix platforms, Windows and Android happily find their respective versions. http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/35... http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/40630/... http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:98: // Run time based aligned memory allocator. Since the alignment allocators may On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > Why do you want the posix_memalign wrapper to be named the same as the > non-allocating class? It seems better to use different names. I suspect the > memalign wrapper should be a function returning a scoped_ptr_malloc<T, > AlignedFree> instead of a class. 1. Type safety. scoped_ptr_malloc prevents you from having type safety based on alignment for methods requiring aligned data. The current method allows you to do stuff like: void Vector_Multiply_SSE2(const AlignedMemory<16>& src, ...); 2. Encapsulation. scoped_ptr_malloc<T, AlignedFree> means every aligned memory instance now knows about AlignedFree, vs simply AlignedMemory<16> input_array_; 3. Simplicity. Having a single AlignedMemory class feels simpler, cleaner than having AlignedMemory and say DynamicAlignedMemory. It occurs to me now, that requiring a type parameter might make this even safer and more readable: e.g. AlignedMemory<float, 16> or AlignedMemory<void, 32>, etc. This would allow us simplify the templates by removing the void_data() and data_as() in favor of just T* data(), const T* data(). Existing static instances would also have a type. WDYT? http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:109: if (!data_) *reinterpret_cast<uint8*>(data_) = 0; \ On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > Assigning to a null pointer is undefined behavior and may be optimized away > instead of causing a crash. Use CHECK instead. jbates was worried about the weight of CHECK/DCHECK here, so I switched to this method. Is that a valid concern? assert() (like used in scoped_ptr) is another option.
http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:55: ptr = memalign(align, size) On 2012/07/20 18:25:28, DaleCurtis wrote: > On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > > http://linux.die.net/man/3/memalign says, "Some systems provide no way to > > reclaim memory allocated with memalign() or valloc() (because one can only > pass > > to free(3) a pointer gotten from malloc(3), while, for example, memalign() > would > > call malloc(3) and then align the obtained value)." Could you link to the > > android documentation saying this is safe? > > Sadly, NDK documentation is incredibly sparse. I tried this on a hunch to get > the android trybot to pass, but have no documentation to back it up and that bot > doesn't run the unit tests (or valgrind for that matter). Do we have any Android > experts in the house that could answer? I downloaded the NDK and grep'd around > without finding anything useful. (no online source search makes me sad :( > > I'm happy turning this into a NOTREACHED() or something similar for now. Actually, NOTREACHED is probably a bad idea, we could hack up a memalign for Android if it's unavailable: #define INTERNAL_ALIGNED_MALLOC(real_ptr, ptr, align, size) do { \ ptr = real_ptr = malloc(size + (align - 1)); \ ptr += align - (reinterpret_cast<uintptr_t>(real_ptr) & (align - 1)); \ } while (0) #define INTERNAL_ALIGNED_FREE(real_ptr, ptr) \ free(real_ptr)
http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:35: #define _XOPEN_SOURCE 600 // for posix_memalign On 2012/07/20 18:25:28, DaleCurtis wrote: > On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > > If _XOPEN_SOURCE was already defined as <600, this will cause a duplicate > > definition warning/error. > > Well if we keep the extern below this isn't really necessary anyways, so I can > remove it. Alternatively, do you have a suggested improvement? Make sure linux and mac set _XOPEN_SOURCE >=600 or _POSIX_C_SOURCE >= 200112L in the build system, and then remove it from here, I think. http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:55: ptr = memalign(align, size) On 2012/07/20 20:33:37, DaleCurtis wrote: > On 2012/07/20 18:25:28, DaleCurtis wrote: > > On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > > > http://linux.die.net/man/3/memalign says, "Some systems provide no way to > > > reclaim memory allocated with memalign() or valloc() (because one can only > > pass > > > to free(3) a pointer gotten from malloc(3), while, for example, memalign() > > would > > > call malloc(3) and then align the obtained value)." Could you link to the > > > android documentation saying this is safe? > > > > Sadly, NDK documentation is incredibly sparse. I tried this on a hunch to get > > the android trybot to pass, but have no documentation to back it up and that > bot > > doesn't run the unit tests (or valgrind for that matter). Do we have any > Android > > experts in the house that could answer? I downloaded the NDK and grep'd around > > without finding anything useful. (no online source search makes me sad :( > > > > I'm happy turning this into a NOTREACHED() or something similar for now. > > Actually, NOTREACHED is probably a bad idea, we could hack up a memalign for > Android if it's unavailable: > > #define INTERNAL_ALIGNED_MALLOC(real_ptr, ptr, align, size) do { \ > ptr = real_ptr = malloc(size + (align - 1)); \ > ptr += align - (reinterpret_cast<uintptr_t>(real_ptr) & (align - 1)); \ > } while (0) > #define INTERNAL_ALIGNED_FREE(real_ptr, ptr) \ > free(real_ptr) Yeah, NOTREACHED would be bad. Your option is an ok last resort, but you need to check with an Android person first to see whether they include posix_memalign or support free(memalign(...)). Will mentions joth@. Bionic claims to use dlmalloc, which does include posix_memalign: ftp://g.oswego.edu/pub/misc/malloc.c. http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine On 2012/07/20 18:25:28, DaleCurtis wrote: > On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > > What error do they give? It's very weird that <stdlib.h> wouldn't be enough > and > > may indicate something else going on. > > "error: ‘posix_memalign’ was not declared in this scope" -- same message on all > posix platforms, Windows and Android happily find their respective versions. > > http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/35... > http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/40630/... Definitely figure out what's going wrong with the trybots rather than declaring a standard library function yourself. http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:98: // Run time based aligned memory allocator. Since the alignment allocators may On 2012/07/20 18:25:28, DaleCurtis wrote: > On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > > Why do you want the posix_memalign wrapper to be named the same as the > > non-allocating class? It seems better to use different names. I suspect the > > memalign wrapper should be a function returning a scoped_ptr_malloc<T, > > AlignedFree> instead of a class. > > 1. Type safety. scoped_ptr_malloc prevents you from having type safety based on > alignment for methods requiring aligned data. The current method allows you to > do stuff like: > > void Vector_Multiply_SSE2(const AlignedMemory<16>& src, ...); What you have doesn't actually work for this because it needs to accept any larger alignment, and it should accept inline storage as an argument. That brings up another question: where's the change that requires this new feature? > 2. Encapsulation. scoped_ptr_malloc<T, AlignedFree> means every aligned memory > instance now knows about AlignedFree, vs simply AlignedMemory<16> input_array_; I don't think this is a big deal since the details are still wrapped up in the AlignedFree type. > 3. Simplicity. Having a single AlignedMemory class feels simpler, cleaner than > having AlignedMemory and say DynamicAlignedMemory. Generally putting two different behaviors under the same name doesn't make the code simpler to use. Here you've hidden the cost of allocating memory, and by increasing the amount of template specialization you made the code harder to read for non-experts. > It occurs to me now, that requiring a type parameter might make this even safer > and more readable: e.g. AlignedMemory<float, 16> or AlignedMemory<void, 32>, > etc. This would allow us simplify the templates by removing the void_data() and > data_as() in favor of just T* data(), const T* data(). Existing static instances > would also have a type. WDYT? A type parameter makes this slightly less general, which is likely to be a bad idea, and likely indicates that the dynamic version shouldn't actually return scoped_ptr_malloc<>. Any typed thing should wrap the raw aligning function. Another reason to keep AlignedMemory as-is is that C++11 defines std::aligned_storage with the same meaning as the current AlignedMemory. C11 defines "void* aligned_alloc(alignment, size)" as the dynamically-allocated version. http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:109: if (!data_) *reinterpret_cast<uint8*>(data_) = 0; \ On 2012/07/20 18:25:28, DaleCurtis wrote: > On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > > Assigning to a null pointer is undefined behavior and may be optimized away > > instead of causing a crash. Use CHECK instead. > > jbates was worried about the weight of CHECK/DCHECK here, so I switched to this > method. Is that a valid concern? assert() (like used in scoped_ptr) is another > option. Anything other than dereferencing null would be fine with me.
+jvoung: Re, http://code.google.com/p/nativeclient/issues/detail?id=2505 we seem to be hitting that issue here. Is adding the extern for posix_memalign the correct solution? (See aligned_memory.cc) Thanks! https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... File base/memory/aligned_memory.h (right): https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... base/memory/aligned_memory.h:35: #define _XOPEN_SOURCE 600 // for posix_memalign On 2012/07/20 23:36:56, Jeffrey Yasskin wrote: > On 2012/07/20 18:25:28, DaleCurtis wrote: > > On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > > > If _XOPEN_SOURCE was already defined as <600, this will cause a duplicate > > > definition warning/error. > > > > Well if we keep the extern below this isn't really necessary anyways, so I can > > remove it. Alternatively, do you have a suggested improvement? > > Make sure linux and mac set _XOPEN_SOURCE >=600 or _POSIX_C_SOURCE >= 200112L in > the build system, and then remove it from here, I think. I'm wary of setting this all over the code base and things seem to compile fine without it, so removed? https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... base/memory/aligned_memory.h:55: ptr = memalign(align, size) On 2012/07/20 23:36:56, Jeffrey Yasskin wrote: > On 2012/07/20 20:33:37, DaleCurtis wrote: > > On 2012/07/20 18:25:28, DaleCurtis wrote: > > > On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > > > > http://linux.die.net/man/3/memalign says, "Some systems provide no way to > > > > reclaim memory allocated with memalign() or valloc() (because one can only > > > pass > > > > to free(3) a pointer gotten from malloc(3), while, for example, memalign() > > > would > > > > call malloc(3) and then align the obtained value)." Could you link to the > > > > android documentation saying this is safe? > > > > > > Sadly, NDK documentation is incredibly sparse. I tried this on a hunch to > get > > > the android trybot to pass, but have no documentation to back it up and that > > bot > > > doesn't run the unit tests (or valgrind for that matter). Do we have any > > Android > > > experts in the house that could answer? I downloaded the NDK and grep'd > around > > > without finding anything useful. (no online source search makes me sad :( > > > > > > I'm happy turning this into a NOTREACHED() or something similar for now. > > > > Actually, NOTREACHED is probably a bad idea, we could hack up a memalign for > > Android if it's unavailable: > > > > #define INTERNAL_ALIGNED_MALLOC(real_ptr, ptr, align, size) do { \ > > ptr = real_ptr = malloc(size + (align - 1)); \ > > ptr += align - (reinterpret_cast<uintptr_t>(real_ptr) & (align - 1)); \ > > } while (0) > > #define INTERNAL_ALIGNED_FREE(real_ptr, ptr) \ > > free(real_ptr) > > Yeah, NOTREACHED would be bad. Your option is an ok last resort, but you need to > check with an Android person first to see whether they include posix_memalign or > support free(memalign(...)). Will mentions joth@. Bionic claims to use dlmalloc, > which does include posix_memalign: ftp://g.oswego.edu/pub/misc/malloc.c. Per https://github.com/android/platform_bionic/commit/85aad909560508410101c18c6ec... aligned_malloc() supports free on Android. While it claims posix_memalign is available, the bots can't seem to find it. https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine On 2012/07/20 23:36:56, Jeffrey Yasskin wrote: > On 2012/07/20 18:25:28, DaleCurtis wrote: > > On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > > > What error do they give? It's very weird that <stdlib.h> wouldn't be enough > > and > > > may indicate something else going on. > > > > "error: ‘posix_memalign’ was not declared in this scope" -- same message on > all > > posix platforms, Windows and Android happily find their respective versions. > > > > > http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/35... > > > http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/40630/... > > Definitely figure out what's going wrong with the trybots rather than declaring > a standard library function yourself. Turns out this is a NaCl issue, http://code.google.com/p/nativeclient/issues/detail?id=2505 +jvoung who filed the issue there. https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... base/memory/aligned_memory.h:98: // Run time based aligned memory allocator. Since the alignment allocators may On 2012/07/20 23:36:56, Jeffrey Yasskin wrote: > On 2012/07/20 18:25:28, DaleCurtis wrote: > > On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > > > Why do you want the posix_memalign wrapper to be named the same as the > > > non-allocating class? It seems better to use different names. I suspect the > > > memalign wrapper should be a function returning a scoped_ptr_malloc<T, > > > AlignedFree> instead of a class. > > > > 1. Type safety. scoped_ptr_malloc prevents you from having type safety based > on > > alignment for methods requiring aligned data. The current method allows you to > > do stuff like: > > > > void Vector_Multiply_SSE2(const AlignedMemory<16>& src, ...); > > What you have doesn't actually work for this because it needs to accept any > larger alignment, and it should accept inline storage as an argument. > > That brings up another question: where's the change that requires this new > feature? > > > 2. Encapsulation. scoped_ptr_malloc<T, AlignedFree> means every aligned memory > > instance now knows about AlignedFree, vs simply AlignedMemory<16> > input_array_; > > I don't think this is a big deal since the details are still wrapped up in the > AlignedFree type. > > > 3. Simplicity. Having a single AlignedMemory class feels simpler, cleaner than > > having AlignedMemory and say DynamicAlignedMemory. > > Generally putting two different behaviors under the same name doesn't make the > code simpler to use. Here you've hidden the cost of allocating memory, and by > increasing the amount of template specialization you made the code harder to > read for non-experts. > > > It occurs to me now, that requiring a type parameter might make this even > safer > > and more readable: e.g. AlignedMemory<float, 16> or AlignedMemory<void, 32>, > > etc. This would allow us simplify the templates by removing the void_data() > and > > data_as() in favor of just T* data(), const T* data(). Existing static > instances > > would also have a type. WDYT? > > A type parameter makes this slightly less general, which is likely to be a bad > idea, and likely indicates that the dynamic version shouldn't actually return > scoped_ptr_malloc<>. Any typed thing should wrap the raw aligning function. > > Another reason to keep AlignedMemory as-is is that C++11 defines > std::aligned_storage with the same meaning as the current AlignedMemory. C11 > defines "void* aligned_alloc(alignment, size)" as the dynamically-allocated > version. After discussing over chat with Jeffrey I came to the conclusion that simply having AllignedAlloc and AlignedFree functions will be easier and cleaner for all involved. https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... base/memory/aligned_memory.h:109: if (!data_) *reinterpret_cast<uint8*>(data_) = 0; \ On 2012/07/20 23:36:56, Jeffrey Yasskin wrote: > On 2012/07/20 18:25:28, DaleCurtis wrote: > > On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > > > Assigning to a null pointer is undefined behavior and may be optimized away > > > instead of causing a crash. Use CHECK instead. > > > > jbates was worried about the weight of CHECK/DCHECK here, so I switched to > this > > method. Is that a valid concern? assert() (like used in scoped_ptr) is another > > option. > > Anything other than dereferencing null would be fine with me. Since this is now in a .cc file, switched to CHECK().
https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... File base/memory/aligned_memory.h (right): https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine On 2012/07/21 03:04:40, DaleCurtis wrote: > On 2012/07/20 23:36:56, Jeffrey Yasskin wrote: > > On 2012/07/20 18:25:28, DaleCurtis wrote: > > > On 2012/07/20 16:41:42, Jeffrey Yasskin wrote: > > > > What error do they give? It's very weird that <stdlib.h> wouldn't be > enough > > > and > > > > may indicate something else going on. > > > > > > "error: ‘posix_memalign’ was not declared in this scope" -- same message on > > all > > > posix platforms, Windows and Android happily find their respective versions. > > > > > > > > > http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/35... > > > > > > http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/40630/... > > > > Definitely figure out what's going wrong with the trybots rather than > declaring > > a standard library function yourself. > > Turns out this is a NaCl issue, > http://code.google.com/p/nativeclient/issues/detail?id=2505 > > +jvoung who filed the issue there. Hmm, so this is while building the nacl-ized version of base, using the newlib toolchain. newlib doesn't have that function, but we've ditched the newlib malloc and used dlmalloc, so the nacl libary does technically have posix_memalign(). You can #ifdef __native_client__ to make this more specific / descriptive (or maybe there is an OS_NACL). Before you do this though, unfortunately, there is also a bug in dlmalloc's posix_memalign() (see comment 3 of the bug). I've reported that to doug lea, but I'm not sure when he will roll in a fix, and I haven't patched our version of newlib yet (and haven't patched any headers). What DOES work though, is memalign(). It might even be in the newlib header. Since you already use memalign() for android, can you do the same for NaCl?
http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine On 2012/07/21 04:45:26, jvoung (chromium) wrote: > > Hmm, so this is while building the nacl-ized version of base, using the newlib > toolchain. > > newlib doesn't have that function, but we've ditched the newlib malloc and used > dlmalloc, so the nacl libary does technically have posix_memalign(). > > You can #ifdef __native_client__ to make this more specific / descriptive (or > maybe there is an OS_NACL). > > > Before you do this though, unfortunately, there is also a bug in dlmalloc's > posix_memalign() (see comment 3 of the bug). I've reported that to doug lea, > but I'm not sure when he will roll in a fix, and I haven't patched our version > of newlib yet (and haven't patched any headers). > > What DOES work though, is memalign(). It might even be in the newlib header. > Since you already use memalign() for android, can you do the same for NaCl? > Thanks for the quick response! SGTM. I'll add OS_NACL to the memalign() #if check in the next patch set after jyasskin makes a pass.
https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... File base/memory/aligned_memory.h (right): https://chromiumcodereview.appspot.com/10796020/diff/5018/base/memory/aligned... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine On 2012/07/21 04:45:26, jvoung (chromium) wrote: > On 2012/07/21 03:04:40, DaleCurtis wrote: > > On 2012/07/20 23:36:56, Jeffrey Yasskin wrote: > > > Definitely figure out what's going wrong with the trybots rather than > > declaring > > > a standard library function yourself. > > > > Turns out this is a NaCl issue, > > http://code.google.com/p/nativeclient/issues/detail?id=2505 > > > > +jvoung who filed the issue there. > > Hmm, so this is while building the nacl-ized version of base, using the newlib > toolchain. > > newlib doesn't have that function, but we've ditched the newlib malloc and used > dlmalloc, so the nacl libary does technically have posix_memalign(). > > You can #ifdef __native_client__ to make this more specific / descriptive (or > maybe there is an OS_NACL). Thanks for the reply. Note that AFAIK Dale wasn't trying to build this code for NaCl; he was just trying to build it on the linux and mac trybots. Are the trybots actually building against NaCl's libc instead of their target's libc? > Before you do this though, unfortunately, there is also a bug in dlmalloc's > posix_memalign() (see comment 3 of the bug). I've reported that to doug lea, > but I'm not sure when he will roll in a fix, and I haven't patched our version > of newlib yet (and haven't patched any headers). > > What DOES work though, is memalign(). It might even be in the newlib header. > Since you already use memalign() for android, can you do the same for NaCl? Is there documentation for NaCl saying that its memalign() returns pointers that can be passed to free()? I'd like a link to that documentation here. https://chromiumcodereview.appspot.com/10796020/diff/34002/base/memory/aligne... File base/memory/aligned_memory.h (right): https://chromiumcodereview.appspot.com/10796020/diff/34002/base/memory/aligne... base/memory/aligned_memory.h:50: template <size_t Size, size_t ByteAlignment> Could you revert the argument order flip, now that it's no longer necessary? https://chromiumcodereview.appspot.com/10796020/diff/34002/base/memory/aligne... base/memory/aligned_memory.h:93: BASE_EXPORT void* AlignedAlloc(size_t size, size_t alignment); Please make this signature match C11's aligned_alloc(), which is declared as "void *aligned_alloc(size_t alignment, size_t size);" https://chromiumcodereview.appspot.com/10796020/diff/34002/base/memory/aligne... base/memory/aligned_memory.h:104: class BASE_EXPORT ScopedAlignedFree { This name seems awkward, but I don't have a concrete suggestion for a better one.
http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.h:50: template <size_t Size, size_t ByteAlignment> On 2012/07/21 20:38:49, Jeffrey Yasskin wrote: > Could you revert the argument order flip, now that it's no longer necessary? I already did; this is the original order. http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.h:93: BASE_EXPORT void* AlignedAlloc(size_t size, size_t alignment); On 2012/07/21 20:38:49, Jeffrey Yasskin wrote: > Please make this signature match C11's aligned_alloc(), which is declared as > "void *aligned_alloc(size_t alignment, size_t size);" I used this since it matches the original AlignedMemory order above, it seems reasonable to keep them the same. Do you want to flip them both? http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.h:104: class BASE_EXPORT ScopedAlignedFree { On 2012/07/21 20:38:49, Jeffrey Yasskin wrote: > This name seems awkward, but I don't have a concrete suggestion for a better > one. AlignedFreeHelper, AlignedFreeScoper? The default one in scoped_ptr is called ScopedPtrMallocFree, so this could be ScopedPtrAlignedFree.
http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory.cc File base/memory/aligned_memory.cc (right): http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.cc:19: DCHECK_GT(size, static_cast<size_t>(0)); FWIW, "0U" usually works instead of static_cast<size_t>, unless there's a template being instantiated that wants the types to actually be the same. http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.cc:26: ptr = memalign(alignment, size); I do want to find some sort of link to documentation saying memalign->free is fine within Android. http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.cc:34: CHECK(ptr) << "If you crashed here, your aligned allocation is incorrect."; It might be helpful to include 'size' and 'alignment' in the error message here. http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.h:50: template <size_t Size, size_t ByteAlignment> On 2012/07/21 22:18:41, DaleCurtis wrote: > On 2012/07/21 20:38:49, Jeffrey Yasskin wrote: > > Could you revert the argument order flip, now that it's no longer necessary? > > I already did; this is the original order. Oops, sorry, I compared the wrong versions of the file. http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.h:93: BASE_EXPORT void* AlignedAlloc(size_t size, size_t alignment); On 2012/07/21 22:18:41, DaleCurtis wrote: > On 2012/07/21 20:38:49, Jeffrey Yasskin wrote: > > Please make this signature match C11's aligned_alloc(), which is declared as > > "void *aligned_alloc(size_t alignment, size_t size);" > > I used this since it matches the original AlignedMemory order above, it seems > reasonable to keep them the same. Do you want to flip them both? It does seem reasonable to keep them the same. Unfortunately, C++11's aligned_storage<> and C11's aligned_alloc() picked opposite orders. Let's get a third opinion from someone with more Chrome experience. http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.h:104: class BASE_EXPORT ScopedAlignedFree { On 2012/07/21 22:18:41, DaleCurtis wrote: > On 2012/07/21 20:38:49, Jeffrey Yasskin wrote: > > This name seems awkward, but I don't have a concrete suggestion for a better > > one. > > AlignedFreeHelper, AlignedFreeScoper? The default one in scoped_ptr is called > ScopedPtrMallocFree, so this could be ScopedPtrAlignedFree. ScopedPtrAlignedFree seems the least bad of those, but it is annoyingly long. No strong opinions from me.
http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine On 2012/07/21 18:30:32, DaleCurtis wrote: > On 2012/07/21 04:45:26, jvoung (chromium) wrote: > > > > Hmm, so this is while building the nacl-ized version of base, using the newlib > > toolchain. > > > > newlib doesn't have that function, but we've ditched the newlib malloc and > used > > dlmalloc, so the nacl libary does technically have posix_memalign(). > > > > You can #ifdef __native_client__ to make this more specific / descriptive (or > > maybe there is an OS_NACL). > > > > > > Before you do this though, unfortunately, there is also a bug in dlmalloc's > > posix_memalign() (see comment 3 of the bug). I've reported that to doug lea, > > but I'm not sure when he will roll in a fix, and I haven't patched our version > > of newlib yet (and haven't patched any headers). > > > > What DOES work though, is memalign(). It might even be in the newlib header. > > Since you already use memalign() for android, can you do the same for NaCl? > > > > Thanks for the quick response! SGTM. I'll add OS_NACL to the memalign() #if > check in the next patch set after jyasskin makes a pass. Looks like the extern definition is still necessary for memalign as well. :( http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/41602/... http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory.cc File base/memory/aligned_memory.cc (right): http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.cc:19: DCHECK_GT(size, static_cast<size_t>(0)); On 2012/07/21 22:42:06, Jeffrey Yasskin wrote: > FWIW, "0U" usually works instead of static_cast<size_t>, unless there's a > template being instantiated that wants the types to actually be the same. Done. http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.cc:26: ptr = memalign(alignment, size); On 2012/07/21 22:42:06, Jeffrey Yasskin wrote: > I do want to find some sort of link to documentation saying memalign->free is > fine within Android. I suspect aside from the commit message I linked previously and the standard "read the code", the only documentation we'll find is what we write here. :) As such, I've added a nice comment for future travelers. http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.cc:34: CHECK(ptr) << "If you crashed here, your aligned allocation is incorrect."; On 2012/07/21 22:42:07, Jeffrey Yasskin wrote: > It might be helpful to include 'size' and 'alignment' in the error message here. Done. http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/34002/base/memory/aligned_memory... base/memory/aligned_memory.h:104: class BASE_EXPORT ScopedAlignedFree { On 2012/07/21 22:42:07, Jeffrey Yasskin wrote: > On 2012/07/21 22:18:41, DaleCurtis wrote: > > On 2012/07/21 20:38:49, Jeffrey Yasskin wrote: > > > This name seems awkward, but I don't have a concrete suggestion for a better > > > one. > > > > AlignedFreeHelper, AlignedFreeScoper? The default one in scoped_ptr is called > > ScopedPtrMallocFree, so this could be ScopedPtrAlignedFree. > > ScopedPtrAlignedFree seems the least bad of those, but it is annoyingly long. No > strong opinions from me. Changed to ScopedPtrAlignedFree for consistency with scoped_ptr.
http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine On 2012/07/22 00:45:40, DaleCurtis wrote: > On 2012/07/21 18:30:32, DaleCurtis wrote: > > On 2012/07/21 04:45:26, jvoung (chromium) wrote: > > > > > > Hmm, so this is while building the nacl-ized version of base, using the > newlib > > > toolchain. > > > > > > newlib doesn't have that function, but we've ditched the newlib malloc and > > used > > > dlmalloc, so the nacl libary does technically have posix_memalign(). > > > > > > You can #ifdef __native_client__ to make this more specific / descriptive > (or > > > maybe there is an OS_NACL). > > > > > > > > > Before you do this though, unfortunately, there is also a bug in dlmalloc's > > > posix_memalign() (see comment 3 of the bug). I've reported that to doug > lea, > > > but I'm not sure when he will roll in a fix, and I haven't patched our > version > > > of newlib yet (and haven't patched any headers). > > > > > > What DOES work though, is memalign(). It might even be in the newlib > header. > > > Since you already use memalign() for android, can you do the same for NaCl? > > > > > > > Thanks for the quick response! SGTM. I'll add OS_NACL to the memalign() #if > > check in the next patch set after jyasskin makes a pass. > > Looks like the extern definition is still necessary for memalign as well. :( > http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/41602/... Actually scratch that, I just forgot the header.
http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine On 2012/07/22 01:16:13, DaleCurtis wrote: > On 2012/07/22 00:45:40, DaleCurtis wrote: > > On 2012/07/21 18:30:32, DaleCurtis wrote: > > > On 2012/07/21 04:45:26, jvoung (chromium) wrote: > > > > > > > > Hmm, so this is while building the nacl-ized version of base, using the > > newlib > > > > toolchain. > > > > > > > > newlib doesn't have that function, but we've ditched the newlib malloc and > > > used > > > > dlmalloc, so the nacl libary does technically have posix_memalign(). > > > > > > > > You can #ifdef __native_client__ to make this more specific / descriptive > > (or > > > > maybe there is an OS_NACL). > > > > > > > > > > > > Before you do this though, unfortunately, there is also a bug in > dlmalloc's > > > > posix_memalign() (see comment 3 of the bug). I've reported that to doug > > lea, > > > > but I'm not sure when he will roll in a fix, and I haven't patched our > > version > > > > of newlib yet (and haven't patched any headers). > > > > > > > > What DOES work though, is memalign(). It might even be in the newlib > > header. > > > > Since you already use memalign() for android, can you do the same for > NaCl? > > > > > > > > > > Thanks for the quick response! SGTM. I'll add OS_NACL to the memalign() #if > > > check in the next patch set after jyasskin makes a pass. > > > > Looks like the extern definition is still necessary for memalign as well. :( > > > http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/41602/... > > Actually scratch that, I just forgot the header. Hrmmm... just checked the newlib sources. I don't think it exports memalign either. http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine On 2012/07/21 20:38:48, Jeffrey Yasskin wrote: > On 2012/07/21 04:45:26, jvoung (chromium) wrote: > > On 2012/07/21 03:04:40, DaleCurtis wrote: > > > On 2012/07/20 23:36:56, Jeffrey Yasskin wrote: > > > > Definitely figure out what's going wrong with the trybots rather than > > > declaring > > > > a standard library function yourself. > > > > > > Turns out this is a NaCl issue, > > > http://code.google.com/p/nativeclient/issues/detail?id=2505 > > > > > > +jvoung who filed the issue there. > > > > Hmm, so this is while building the nacl-ized version of base, using the newlib > > toolchain. > > > > newlib doesn't have that function, but we've ditched the newlib malloc and > used > > dlmalloc, so the nacl libary does technically have posix_memalign(). > > > > You can #ifdef __native_client__ to make this more specific / descriptive (or > > maybe there is an OS_NACL). > > Thanks for the reply. Note that AFAIK Dale wasn't trying to build this code for > NaCl; he was just trying to build it on the linux and mac trybots. Are the > trybots actually building against NaCl's libc instead of their target's libc? > My guess is that the trybots are also building against NaCl's libc because of some of the NaCl pepper proxy. That might be a prerequisite for running NaCl integration tests, but I'm not sure (could ask bbudge or dmichael). > > Before you do this though, unfortunately, there is also a bug in dlmalloc's > > posix_memalign() (see comment 3 of the bug). I've reported that to doug lea, > > but I'm not sure when he will roll in a fix, and I haven't patched our version > > of newlib yet (and haven't patched any headers). > > > > What DOES work though, is memalign(). It might even be in the newlib header. > > Since you already use memalign() for android, can you do the same for NaCl? > > Is there documentation for NaCl saying that its memalign() returns pointers that > can be passed to free()? I'd like a link to that documentation here. I don't think there is actual documentation. NaCl has two libc choices right now: (1) dlmalloc + newlib, or (2) glibc. Under the page you listed glibc (2) is in the clear. For (1), the dlmalloc source (around line 5248) shows that both memalign() and posix_memalign() return memory from an internal_memalign() function. Since you can free memory from posix_memalign(), I think you can free memory from internal_memalign() and thus from memalign() too.
http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine On 2012/07/23 17:06:36, jvoung (chromium) wrote: > On 2012/07/21 20:38:48, Jeffrey Yasskin wrote: > > On 2012/07/21 04:45:26, jvoung (chromium) wrote: > > > On 2012/07/21 03:04:40, DaleCurtis wrote: > > > > On 2012/07/20 23:36:56, Jeffrey Yasskin wrote: > > > > > Definitely figure out what's going wrong with the trybots rather than > > > > declaring > > > > > a standard library function yourself. > > > > > > > > Turns out this is a NaCl issue, > > > > http://code.google.com/p/nativeclient/issues/detail?id=2505 > > > > > > > > +jvoung who filed the issue there. > > > > > > Hmm, so this is while building the nacl-ized version of base, using the > newlib > > > toolchain. > > > > > > newlib doesn't have that function, but we've ditched the newlib malloc and > > used > > > dlmalloc, so the nacl libary does technically have posix_memalign(). > > > > > > You can #ifdef __native_client__ to make this more specific / descriptive > (or > > > maybe there is an OS_NACL). > > > > Thanks for the reply. Note that AFAIK Dale wasn't trying to build this code > for > > NaCl; he was just trying to build it on the linux and mac trybots. Are the > > trybots actually building against NaCl's libc instead of their target's libc? > > > > > My guess is that the trybots are also building against NaCl's libc because of > some of the NaCl pepper proxy. That might be a prerequisite for running NaCl > integration tests, but I'm not sure (could ask bbudge or dmichael). We're building some bits of Chrome in base, ipc, and gpu [possibly others] with the newlib NaCl toolchain now. This is part of the work for the NaCl ppapi proxy, which is going to transition to using Chrome IPC "soon". We need to use newlib, because this code goes in to the NaCl IRT, which must use newlib. HTH. > > > > > Before you do this though, unfortunately, there is also a bug in dlmalloc's > > > posix_memalign() (see comment 3 of the bug). I've reported that to doug > lea, > > > but I'm not sure when he will roll in a fix, and I haven't patched our > version > > > of newlib yet (and haven't patched any headers). > > > > > > What DOES work though, is memalign(). It might even be in the newlib > header. > > > Since you already use memalign() for android, can you do the same for NaCl? > > > > Is there documentation for NaCl saying that its memalign() returns pointers > that > > can be passed to free()? I'd like a link to that documentation here. > > > I don't think there is actual documentation. NaCl has two libc choices right > now: (1) dlmalloc + newlib, or (2) glibc. > > Under the page you listed glibc (2) is in the clear. > > For (1), the dlmalloc source (around line 5248) shows that both memalign() and > posix_memalign() return memory from an internal_memalign() function. Since you > can free memory from posix_memalign(), I think you can free memory from > internal_memalign() and thus from memalign() too. >
http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/5018/base/memory/aligned_memory.... base/memory/aligned_memory.h:58: // Buildbots can find the posix_memalign definition. Everything builds fine On 2012/07/23 17:06:36, jvoung (chromium) wrote: > On 2012/07/22 01:16:13, DaleCurtis wrote: > > On 2012/07/22 00:45:40, DaleCurtis wrote: > > > On 2012/07/21 18:30:32, DaleCurtis wrote: > > > > On 2012/07/21 04:45:26, jvoung (chromium) wrote: > > > > > > > > > > Hmm, so this is while building the nacl-ized version of base, using the > > > newlib > > > > > toolchain. > > > > > > > > > > newlib doesn't have that function, but we've ditched the newlib malloc > and > > > > used > > > > > dlmalloc, so the nacl libary does technically have posix_memalign(). > > > > > > > > > > You can #ifdef __native_client__ to make this more specific / > descriptive > > > (or > > > > > maybe there is an OS_NACL). > > > > > > > > > > > > > > > Before you do this though, unfortunately, there is also a bug in > > dlmalloc's > > > > > posix_memalign() (see comment 3 of the bug). I've reported that to doug > > > lea, > > > > > but I'm not sure when he will roll in a fix, and I haven't patched our > > > version > > > > > of newlib yet (and haven't patched any headers). > > > > > > > > > > What DOES work though, is memalign(). It might even be in the newlib > > > header. > > > > > Since you already use memalign() for android, can you do the same for > > NaCl? > > > > > > > > > > > > > Thanks for the quick response! SGTM. I'll add OS_NACL to the memalign() > #if > > > > check in the next patch set after jyasskin makes a pass. > > > > > > Looks like the extern definition is still necessary for memalign as well. :( > > > > > > http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/41602/... > > > > Actually scratch that, I just forgot the header. > > Hrmmm... just checked the newlib sources. I don't think it exports memalign > either. n/m, it's in malloc.h there
LGTM aside from the reinterpret_cast<T*>s and hopefully the bug filing. Thanks for bearing with me! http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.cc File base/memory/aligned_memory.cc (right): http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.... base/memory/aligned_memory.cc:24: // memalign() on both platforms returns pointers which can safely be used with I've seen too many incorrect claims of this form to believe them anymore. ;) ("This data race is totally safe I promise!!1! ... *crash*") Since there's no documentation, would it be too much to ask you to file bugs against NaCl and Android to _add_ documentation, and link to them from here? Then at least future maintainers have somewhere to go check to see if the claim's become incorrect in the mean time. If I'm overstepping my limits as a chrome reviewer, it's ok to tell me to buzz off on this. :) http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.... base/memory/aligned_memory.h:32: // reinterpret_cast<float*>(AlignedAlloc(size, alignment))); These reinterpret_casts only need to be static_casts because they're void*->T*, and _should_ be static_casts to make it clearer that every use of reinterpret_cast<T*> is a potential bug. http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory_... File base/memory/aligned_memory_unittest.cc (right): http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory_... base/memory/aligned_memory_unittest.cc:78: reinterpret_cast<float*>(base::AlignedAlloc(8, 8))); static_cast here too.
Where's the hack around the MSVC __alignof() issue referenced in the description? Is that still there? If so, I missed it :( http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.cc File base/memory/aligned_memory.cc (right): http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.... base/memory/aligned_memory.cc:7: #if defined(OS_ANDROID) || defined(OS_NACL) As per Chromium style guide, please move platform specific #includes after all the cross-platform ones. http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.... base/memory/aligned_memory.cc:35: CHECK(ptr) << "If you crashed here, your aligned allocation is incorrect: " I know Jeffrey asked for more info here, but in order to avoid binary bloat, please remove the message portion here. Alternatively, only keep those strings in debug mode. http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.... base/memory/aligned_memory.h:37: #if defined(COMPILER_MSVC) Platform specific #includes should be behind the cross-platform #includes, as per Chromium style guide, unless you have a special need for these to be first (AFAICT, not true).
http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.cc File base/memory/aligned_memory.cc (right): http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.... base/memory/aligned_memory.cc:7: #if defined(OS_ANDROID) || defined(OS_NACL) On 2012/07/23 18:48:45, willchan wrote: > As per Chromium style guide, please move platform specific #includes after all > the cross-platform ones. Done. http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.... base/memory/aligned_memory.cc:24: // memalign() on both platforms returns pointers which can safely be used with On 2012/07/23 18:34:40, Jeffrey Yasskin wrote: > I've seen too many incorrect claims of this form to believe them anymore. ;) > ("This data race is totally safe I promise!!1! ... *crash*") > > Since there's no documentation, would it be too much to ask you to file bugs > against NaCl and Android to _add_ documentation, and link to them from here? > Then at least future maintainers have somewhere to go check to see if the > claim's become incorrect in the mean time. If I'm overstepping my limits as a > chrome reviewer, it's ok to tell me to buzz off on this. :) The absolute worst case here is free() silently accepting non-malloc'd addresses. The more likely case seems to be that free() will crash immediately if this statement is not true. In any case, I'll file documentation bugs w/ the respective projects as soon as GoogleCode goes out of read-only mode. http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.... base/memory/aligned_memory.cc:35: CHECK(ptr) << "If you crashed here, your aligned allocation is incorrect: " On 2012/07/23 18:48:45, willchan wrote: > I know Jeffrey asked for more info here, but in order to avoid binary bloat, > please remove the message portion here. Alternatively, only keep those strings > in debug mode. Done. http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.h File base/memory/aligned_memory.h (right): http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.... base/memory/aligned_memory.h:32: // reinterpret_cast<float*>(AlignedAlloc(size, alignment))); On 2012/07/23 18:34:40, Jeffrey Yasskin wrote: > These reinterpret_casts only need to be static_casts because they're void*->T*, > and _should_ be static_casts to make it clearer that every use of > reinterpret_cast<T*> is a potential bug. Fixed all over. http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.... base/memory/aligned_memory.h:37: #if defined(COMPILER_MSVC) On 2012/07/23 18:48:45, willchan wrote: > Platform specific #includes should be behind the cross-platform #includes, as > per Chromium style guide, unless you have a special need for these to be first > (AFAICT, not true). Done. Causes a lint error though. http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory_... File base/memory/aligned_memory_unittest.cc (right): http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory_... base/memory/aligned_memory_unittest.cc:78: reinterpret_cast<float*>(base::AlignedAlloc(8, 8))); On 2012/07/23 18:34:40, Jeffrey Yasskin wrote: > static_cast here too. Done.
http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.cc File base/memory/aligned_memory.cc (right): http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.... base/memory/aligned_memory.cc:24: // memalign() on both platforms returns pointers which can safely be used with On 2012/07/23 19:04:27, DaleCurtis wrote: > On 2012/07/23 18:34:40, Jeffrey Yasskin wrote: > > I've seen too many incorrect claims of this form to believe them anymore. ;) > > ("This data race is totally safe I promise!!1! ... *crash*") > > > > Since there's no documentation, would it be too much to ask you to file bugs > > against NaCl and Android to _add_ documentation, and link to them from here? > > Then at least future maintainers have somewhere to go check to see if the > > claim's become incorrect in the mean time. If I'm overstepping my limits as a > > chrome reviewer, it's ok to tell me to buzz off on this. :) > > The absolute worst case here is free() silently accepting non-malloc'd > addresses. The more likely case seems to be that free() will crash immediately > if this statement is not true. AIUI, passing non-malloc'd addresses to free() is a security hole. > > In any case, I'll file documentation bugs w/ the respective projects as soon as > GoogleCode goes out of read-only mode.
http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.cc File base/memory/aligned_memory.cc (right): http://codereview.chromium.org/10796020/diff/7037/base/memory/aligned_memory.... base/memory/aligned_memory.cc:24: // memalign() on both platforms returns pointers which can safely be used with On 2012/07/23 19:15:44, willchan wrote: > On 2012/07/23 19:04:27, DaleCurtis wrote: > > On 2012/07/23 18:34:40, Jeffrey Yasskin wrote: > > > I've seen too many incorrect claims of this form to believe them anymore. ;) > > > ("This data race is totally safe I promise!!1! ... *crash*") > > > > > > Since there's no documentation, would it be too much to ask you to file bugs > > > against NaCl and Android to _add_ documentation, and link to them from here? > > > Then at least future maintainers have somewhere to go check to see if the > > > claim's become incorrect in the mean time. If I'm overstepping my limits as > a > > > chrome reviewer, it's ok to tell me to buzz off on this. :) > > > > The absolute worst case here is free() silently accepting non-malloc'd > > addresses. The more likely case seems to be that free() will crash > immediately > > if this statement is not true. > > AIUI, passing non-malloc'd addresses to free() is a security hole. > > > > > In any case, I'll file documentation bugs w/ the respective projects as soon > as > > GoogleCode goes out of read-only mode. > We have confirmation from NaCl and Android that this is safe (via jvoung and via commit message / code in dlmalloc / Bionic), just no user documentation; so my comment was more of a "what if". You're correct though on the security implications, my point was that I suspect the likely case is fail fast.
LGTM, thanks for the change. Thanks Jeffrey for doing the primary review. http://codereview.chromium.org/10796020/diff/10043/base/compiler_specific.h File base/compiler_specific.h (right): http://codereview.chromium.org/10796020/diff/10043/base/compiler_specific.h#n... base/compiler_specific.h:119: // http://goo.gl/isH0C Minor nit, I think there's no need for a shortened URL. Just provide the full one. svn is not Twitter :P http://codereview.chromium.org/10796020/diff/10043/base/memory/aligned_memory.cc File base/memory/aligned_memory.cc (right): http://codereview.chromium.org/10796020/diff/10043/base/memory/aligned_memory... base/memory/aligned_memory.cc:36: // Instead of CHECK(ptr) << msg, avoid binary bloat by moving msg into DLOG. No need for this comment. It's just a general principle that should be followed (I don't think we want this type of comment for every DLOG, just keep educating people and updating online docs). http://codereview.chromium.org/10796020/diff/10043/base/memory/aligned_memory... base/memory/aligned_memory.cc:38: << "size=" << size << ", alignment=" << alignment; << should line up with previous line, as per Google style guide.
Thanks for the reviews everyone. I've added links to the bugs for more documentation on the Android and NaCl projects as well as fixed the last of the comments. Just to be sure, I also double checked with mmentovai that posix_memalign was safe to use now that we're targeting 10.6. CQ'ing. http://codereview.chromium.org/10796020/diff/10043/base/compiler_specific.h File base/compiler_specific.h (right): http://codereview.chromium.org/10796020/diff/10043/base/compiler_specific.h#n... base/compiler_specific.h:119: // http://goo.gl/isH0C On 2012/07/23 20:37:18, willchan wrote: > Minor nit, I think there's no need for a shortened URL. Just provide the full > one. svn is not Twitter :P Done. I'll forward any hate mail I get for a 134 column line to you :) http://codereview.chromium.org/10796020/diff/10043/base/memory/aligned_memory.cc File base/memory/aligned_memory.cc (right): http://codereview.chromium.org/10796020/diff/10043/base/memory/aligned_memory... base/memory/aligned_memory.cc:36: // Instead of CHECK(ptr) << msg, avoid binary bloat by moving msg into DLOG. On 2012/07/23 20:37:18, willchan wrote: > No need for this comment. It's just a general principle that should be followed > (I don't think we want this type of comment for every DLOG, just keep educating > people and updating online docs). Done. http://codereview.chromium.org/10796020/diff/10043/base/memory/aligned_memory... base/memory/aligned_memory.cc:38: << "size=" << size << ", alignment=" << alignment; On 2012/07/23 20:37:18, willchan wrote: > << should line up with previous line, as per Google style guide. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10796020/7040
Presubmit check for 10796020-7040 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). base/compiler_specific.h, line 119, 133 chars Presubmit checks took 1.6s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10796020/11029
Change committed as 147988
one comment on the final changes. might be worth a followup patch? https://chromiumcodereview.appspot.com/10796020/diff/7040/base/memory/aligned... File base/memory/aligned_memory.h (right): https://chromiumcodereview.appspot.com/10796020/diff/7040/base/memory/aligned... base/memory/aligned_memory.h:69: ALIGNAS(byte_alignment) uint8 data_[Size]; \ Making this private actually changes AlignedMemory from POD to non-POD. I'm not sure but this may introduce some unexpected runtime initialization when this is used statically. Unless you needed non-POD I would change it back.
On 2012/07/24 01:30:23, jbates wrote: > one comment on the final changes. might be worth a followup patch? > > https://chromiumcodereview.appspot.com/10796020/diff/7040/base/memory/aligned... > File base/memory/aligned_memory.h (right): > > https://chromiumcodereview.appspot.com/10796020/diff/7040/base/memory/aligned... > base/memory/aligned_memory.h:69: ALIGNAS(byte_alignment) uint8 data_[Size]; \ > Making this private actually changes AlignedMemory from POD to non-POD. I'm not > sure but this may introduce some unexpected runtime initialization when this is > used statically. Unless you needed non-POD I would change it back. Got reverted due to making the NaCl command line too long, so I'll fix it once it lands again. https://code.google.com/p/chromium/issues/detail?id=138653
On 2012/07/23 21:17:53, I haz the power (commit-bot) wrote: > Presubmit check for 10796020-7040 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit Warnings ** > Found lines longer than 80 characters (first 5 shown). > base/compiler_specific.h, line 119, 133 chars > > Presubmit checks took 1.6s to calculate. FTR, if you look at the check code, http://src.chromium.org/viewvc/chrome/trunk/tools/depot_tools/presubmit_canne... you'll see that the hard limit is 120 chars: # Hard line length limit at 50% more. extra_maxlen = file_maxlen * 3 / 2 There is no work around it for now.
Thx for the explanation.
Relanding now that https://src.chromium.org/viewvc/chrome?view=rev&revision=148373 is fixed. This version include the revert of AlignedMemory to a POD type.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10796020/36001
Change committed as 148483 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
