On Wed, Jan 13, 2010 at 8:07 AM, Antoine Labour <pi...@google.com> wrote:
>
> On Tue, Jan 12, 2010 at 10:15 AM, Craig Schlenter
> <craig.schlen...@chromium.org> wrote:
>>
>> On Tue, Jan 12, 2010 at 7:13 PM, Evan Martin <e...@chromium.org> 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: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
