I put a drive-by comment on the bug, but here's my understanding: gcc 4.4 now has -fstrict-aliasing on by default, whereas it was off by default in 4.2.
gcc's behaviour, while annoying, is actually correct in this case: the C++ spec says that pointers to different types may not alias, and gcc will generate code that assumes they don't (eg., if pointer A and pointer B point to different types, it may hoist a load from pointer B above a store to pointer A, despite the fact that they may point to the same address). MSVC won't do this. In order to make life easy, we should probably explicitly turn on -fstrict-aliasing or -fno-strict-aliasing everywhere, so that both gcc 4.2 and 4.4 generate the same code in this case. My 2c: -fno-strict-aliasing is probably the better choice: 1) It won't require fixing a bunch of obscure bugs with even more obscure hacks (eg., unions). 2) Most of the supposed performance advantage of strict aliasing rules is probably taken care of by memory disambiguation<http://en.wikipedia.org/wiki/Memory_disambiguation>in modern (ie., Core2 and later) CPUs. And since -fno-strict-aliasing was the default on gcc 4.2, we were living without any supposed perf improvement until now anyway, AFAIK. As another datapoint, AFAIK, the Linux kernel is compiled with -fno-strict-aliasing as well. Stephen (And some day, maybe C++ will get a real restrict keyword, like C99...) On Wed, Jan 13, 2010 at 1:37 AM, Craig Schlenter < [email protected]> wrote: > On Wed, Jan 13, 2010 at 8:07 AM, Antoine Labour <[email protected]> wrote: > > > > On Tue, Jan 12, 2010 at 10:15 AM, Craig Schlenter > > <[email protected]> wrote: > >> > >> On Tue, Jan 12, 2010 at 7:13 PM, Evan Martin <[email protected]> wrote: > >> > In this bug > >> > http://code.google.com/p/chromium/issues/detail?id=28749 > >> > It seems we're running afoul of a more finicky compiler not liking > >> > some tricks we're playing with objects and memory in LazyInstance. > >> > (You can skip down to comment #30 or so to get to the meat of it -- > >> > above that point we were still trying to figure out what was causing > >> > the crash.) > >> > > >> > I know enough to say that it sounds like the solution to the problem > >> > should involve placement new, but that is the extent of my C++ skill. > >> > If you are a wizard, could you take a look at the patch Craig posted > >> > on the bug (http://codereview.chromium.org/519045/show) and comment > >> > there? > >> > >> [see base/lazy_instance.{h,c}] > >> > >> One possible way of attacking the problem is to ignore the stuff I > >> posted and just figure out how to plumb the return value from the > >> placement new in New() through to Pointer() and use that return value > >> instead of instance in Pointer() but to do it without bloating up the > >> header file with lots more code. It's the reinterpret_cast in > >> Pointer() that we want to avoid. > >> > >> I actually have a patch that does the ugly thing I suggested in the > >> last comment in rietveld but my brain is coming unstuck trying to work > >> out how to do Darin's suggestion which is likely to be more elegant > >> than my attempts. > >> > >> Any help is welcome :) > > > > In theory chars are ok to alias with anything. > > It may be completely stupid but... technically, &buff_ is not strictly > > speaking a char*, so reinterpret_cast-ing it may confuse the compiler. > > &buff_[0] however is a char* - did we try this trivial change ? > > That makes the compiler toss an aliasing error immediately: > > cc1plus: warnings being treated as errors > base/rand_util_posix.cc: In function ‘uint64 base::RandUint64()’: > base/rand_util_posix.cc:32: error: dereferencing pointer ‘instance’ > does break strict-aliasing rules > base/rand_util_posix.cc:47: note: initialized from here > > Beforehand the compiler didn't warn or error at all despite -Wall -Werror > btw. > but it did detect the problem when I did -Wstrict-aliasing=2 > > I did fiddle with some char options after Dean suggested it but, AFAIK, > casting > any Type * to a char * is allowed but the converse is not allowed (when it > is > dereferenced) in terms of the aliasing rules. > > > Also, from the header description: "It also preallocates the space for > Type, > > as to avoid allocating the Type instance on the heap. This may help with > > the performance of creating the instance, and reducing heap > fragmentation. > > This requires that Type be a complete type so we can determine the > size." > > Have we measured it to be significant enough to make it worth banging our > > collective heads on this giant hack ? > > JamesR also suggested just allocating something from the heap which would, > I agree, solve the problem but I was trying to keep LazyInstance as close > as > possible to it's original form. > > I have something that sort-of works btw. that still keeps the > lazyinstance helper > class around: > > http://codereview.chromium.org/548011 > > Thank you, > > --Craig > > -- > Chromium Developers mailing list: [email protected] > View archives, change email options, or unsubscribe: > http://groups.google.com/group/chromium-dev > -- All truth passes through three stages. First, it is ridiculed. Second, it is violently opposed. Third, it is accepted as being self-evident. -- Schopenhauer
-- Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
