Re: Pack files, standards compliance, and efficiency

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jun 5, 2015, 15:22 UTC
Message-ID: <5571BEA0.2080305@alum.mit.edu>
In-Reply-To: <20150605094545.GB11855@peff.net>

On 06/05/2015 11:45 AM, Jeff King wrote:

Show 21 quoted lines

> On Fri, Jun 05, 2015 at 01:41:21AM +0000, brian m. carlson wrote:
> 
>> However, with the object_id conversion, we run into a problem: casting
>> those unsigned char * values into struct object_id * values is not
>> allowed by the C standard.  There are two possible solutions: copying;
>> and the just-do-it solution, where we cast and hope for the best.
> 
> [...]
> 
> But I'm not even sure that line of thinking is all that interesting.
> Even if we are violating some dark corner of the standard, this
> definitely falls into the "it's useful and works on all sane machines"
> category. We also do much worse things with struct-casting mmap'd data
> elsewhere (e.g., see the use of "struct pack_header"). It works fine in
> practice as long as you are careful about alignment and padding issues.
> 
> So my vote would be to retain the cast. This is very low-level,
> performance-sensitive code. I did some very naive timings and didn't see
> any measurable change from your patch, but I also don't think we are
> seeing a real portability benefit to moving to the copy, so I'd prefer
> to keep the status quo.

I don't know that there would necessarily be problems, but I would worry about code involving structure assignment. For example, suppose the following snippet:

    void f(struct object_id *oid)
    {
        struct object_id oid_copy = *oid;
        /* ... */
    }

The compiler is allowed to implement the copy using instructions that rely on proper alignment. Such code would fail if oid is not properly aligned.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/