This is an archive of the discontinued LLVM Phabricator instance.

[clang] Remove assumption about SourceLocation alignment.

Authored by simon_tatham on Jul 6 2021, 9:28 AM.



This is part of a patch series working towards the ability to make
SourceLocation into a 64-bit type to handle larger translation units.

In ObjCMethodDecl, an array of pointers (parameters) and an array of
SourceLocations are stored end to end in a single allocated piece of
memory. The offset within that memory where the second array begins
was being computed in a really simple way, based on the assumption
that pointers had at least as much alignment requirement as
SourceLocations. But when pointers and SourceLocations can each be
either 32 or 64 bits, this won't be a reliable assumption any more.

This patch separates the two types of data into two separate
allocations, so that there's no need to do a tricky offset computation.

Diff Detail

Event Timeline

simon_tatham requested review of this revision.Jul 6 2021, 9:28 AM
simon_tatham created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2021, 9:28 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
miyuki added a subscriber: miyuki.Jul 6 2021, 10:13 AM
tmatheson added inline comments.

Since we know the number of parameters and number of sellocs when we do the allocation, is there a reason we can't we split it into two separate allocations and reduce the complexity?

simon_tatham edited the summary of this revision. (Show Details)

Split up the allocations as suggested.

... and removed an unused function from the previous version.

simon_tatham added inline comments.Jul 20 2021, 6:23 AM

Oops, before anyone else points it out, this #include is also now unnecessary.

tmatheson accepted this revision.Jul 20 2021, 6:26 AM
tmatheson added a subscriber: efriedma.

LGTM, but it would be good to have someone else comment on the increased number of allocations (maybe @rsmith or @efriedma?)


I don't think you need the const_cast

This revision is now accepted and ready to land.Jul 20 2021, 6:26 AM
simon_tatham added inline comments.Jul 20 2021, 7:01 AM

I put it in because I got a compile error otherwise!

It's because of the double indirection. You don't need a cast to turn a Foo * into a const Foo *. But here, we're turning a Foo * into a const Bar *, where Foo and Bar are pointer types to things of different constness.

Addressed two nits.


... ok, no, removing the const_cast now, I don't get a compile error. In that case there must have been something else weird about the version of the code where I had to add it, but I can't reconstruct what.

lebedev.ri resigned from this revision.Jan 12 2023, 5:42 PM

This review seems to be stuck/dead, consider abandoning if no longer relevant.

Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 5:42 PM
Herald added a subscriber: StephenFan. · View Herald Transcript