This is an archive of the discontinued LLVM Phabricator instance.

Changing @llvm.objectsize(i8*, i1) to @llvm.objectsize(i8*, i8)
AbandonedPublic

Authored by george.burgess.iv on Aug 25 2015, 7:35 PM.

Details

Reviewers
echristo
Summary

I'm not sure who to tag for review due to the breadth of this change, so I'm seeing if there are any kind souls out there. :)

In order to make __builtin_object_size do a better job, we need to enable @llvm.objectsize to give more accurate answers. Currently, @llvm.objectsize takes an i1 Min argument instead of a Type, so whether or not we want information about a subobject is lost. Currently, this preference is mostly[1] useless to LLVM, because we can't rely on LLVM's type information to determine the size of a subobject. However, as is alluded to in the diff, the current plan is to use metadata in order to pass this information along so LLVM can be more accurate.

Full proposal for the metadata is here: https://docs.google.com/document/d/1D5GibUI2RCCfa3g1zb5-3a7-l7nY1-tm3jrYciLP8dI/edit?usp=sharing

Accompanying patch for clang will be posted soon.

[1] - If Type = 3, this information is highly relevant, because LLVM can only return 0 in this case. Currently, Clang handles this for LLVM by substituting 0 where it would normally emit @llvm.objectsize(%ptr, 3).

Diff Detail

Event Timeline

george.burgess.iv retitled this revision from to Changing @llvm.objectsize(i8*, i1) to @llvm.objectsize(i8*, i8).
george.burgess.iv updated this object.
george.burgess.iv added a subscriber: llvm-commits.
vsk added a subscriber: vsk.Aug 26 2015, 5:41 PM

I have a few nits and two questions (first question inline).

Second question: I notice that we're changing objectsize(p, 1) to objectsize(p, 2) in the tests. Are existing users of objectsize(p, typ) affected by this, and if so, can we warn them?

thanks

docs/LangRef.rst
4187

&b[0] ?

lib/CodeGen/CodeGenPrepare.cpp
1364

Could you use int for this? It's a nit, but the value isn't strictly binary.

test/CodeGen/Generic/object-size-type.ll
26

Sorry, I may be missing something basic but I don't understand this result. Type=3 should mean find a lower bound of the size from %i to the closest enclosing array. Shouldn't that also be 4?

In your doc, I see: "If it can’t find these tags on potential sources of Ptr, it answers conservatively". Is the problem here that we don't have metadata on the gep?

37

Is it possible to add CHECKs here for the GEP metadata you expect? It'd be good to have at least one check for the 4 different types.

You need auto-upgrade logic to transition old users to the new intrinsic.

george.burgess.iv marked 2 inline comments as done.

Fixed nits + added auto upgrade logic.

In response to comments:

In D12351#233926, @vsk wrote:

I notice that we're changing objectsize(p, 1) to objectsize(p, 2) in the tests. Are existing users of objectsize(p, typ) affected by this, and if so, can we warn them?

First, thanks for the feedback!

Pre-patch objectsize(p, 1) operates differently than post-patch objectsize(p, 1), yes. I'm happy to warn users of the change -- is there an official method for doing so, or will a post to llvm-dev suffice? (FWIW, there's a patch to update Clang to use the new intrinsic @D12352 as well)

You need auto-upgrade logic to transition old users to the new intrinsic.

That's a thing? That's kind of awesome. Thanks for the heads up! :) Is there a preferred way to test new auto-upgrade code, or does "I ran opt on a pre-patch file and the output LGTM for post-patch code" suffice?

test/CodeGen/Generic/object-size-type.ll
26

Type=3 should mean find a lower bound of the size from %i to the closest enclosing array

Not exactly. Type=3 finds the lower bound of the size of the closest source-level subobject. If this subobject is part of an array, then it will get the # of bytes remaining in the array. This exception does not apply for containers of the subobject.

Is the problem here that we don't have metadata on the gep?

Yup! The issue is a combination of two things:

  • We're asking for a lower-bound, so higher-than-optimal answers are not allowed (the opposite is true with Type=1)
  • We have no clue what the source-level types are, and are being asked for the size of a subobject. Consider:
struct Foo { char c; };
struct { struct Foo[8] fs; } b;
__builtin_object_size(&b.fs[4].c, 3); // Pretend this slipped through from clang -- currently clang can answer this correctly.

In this case, the optimal result of BOS is 1, because sizeof(b.fs[4].c) == 1. To your point, 4 would be correct if we changed the BOS call to:

__builtin_object_size(&b.fs[4], 3);
37

I can add checks and make the test an XFAIL if that would be preferable. This patch just brings us to using an i8 for Type. Patch for handling the metadata appropriately is in the works. :)

I left some comments in the design doc.

David

george.burgess.iv abandoned this revision.Dec 4 2015, 10:12 AM

Patch is no longer necessary -- clang can handle __builtin_object_size well enough on its own for type={1,3}.