[cfe-dev] Patch for review: enhancement to __builtin_strlen

[Andy Gibbs]

On Friday, June 22, 2012 12:13 AM, Richard Smith wrote:
> I think the patch is actually too conservative. For instance, I would
> like for the following to work:
>
>   constexpr const char *p = "foobar";
>   constexpr int n = __builtin_strlen(p);
>
> More generally, the builtin should behave as if it were defined as:
>
>  constexpr size_t __builtin_strlen(const char *p) { return *p ? 1 +
> __builtin_strlen(p+1) : 0; }
>
> ... except that it should be faster (and not limited by the constexpr
> recursion depth limit).

Ok, attached then is a first patch towards this end.  It works in most
scenarios, and I have found only one real place where it doesn't, which
is using __builtin_strlen inside a constexpr function with a function
parameter as the expression, but I am pretty sure this is not a fault
in my implementation, but rather that __builtin_strlen as it currently
stands doesn't work inside constexpr functions.  I'll look into it.

I'd like to see if we can get this first patch committed, then I can
look at doing the next stage (otherwise, I just end up with massive
sets of patches that never get committed!...)

I haven't followed exactly your suggestions, but maybe you will spot
some of them in the code anyway.  I actually made the bold step of
splitting out the functionality into its own function, namely
Expr::EvaluateAsString which then returns a folded, truncated
std::string result on success.  __builtin_strlen then calls this
and returns the length of this string.

The approach I took was to consider that the StringLiteral (and I
extended it to include ObjCStringLiteral) is the standard case and
is therefore treated as a first-class citizen, and std::string is
extracted directly.

If it is not a StringLiteral then I check that the expression is either
a const char* or const char[n] type.  I kept with EvaluateAsRValue,
since this works adequately and, I'm afraid to say, I couldn't get
EvaluatePointer to work at all.  The generated APValue result will then
be either an Array or an LValue kind.  If it is an LValue kind, then
it is examined to see whether it is a StringLiteral expression or a
VarDecl.  The former drops out easily, the latter is then evaluated
(in most cases this is already done and it is the cached value that is
returned), and will in most cases drop out as an Array APValue.  And,
finally, an Array kind is copied into std::string and returned if it
is null terminated.

Array and pointer offsets are handled, as is truncation at the first
null character following the offset.

Of your corner cases, I've supported:

constexpr char c = 0;
__builtin_strlen(&c);

The other, i.e.,

struct X { char a; };
__builtin_strlen(&x.c);

I haven't supported in this patch.  I have a mostly working solution
here, but it needs more thought.  For example, is the above enough?
If it is, then like the first corner case, it can only be evaluated
where the value is 0 (which doesn't have many applications!).  But
if you want the following to work...

struct X { int a; char b; char c; char d; float e; };
constexpr X x = { 1234, 'a', 'b', 0, 5.3f }
__builtin_strlen(&x.b);

then I need to think about packing, making sure that it only moves
across types that are char, and not any other types (due to byte
ordering on the target), etc. etc.

Beyond this, it would be as well to point out that I don't think
that you can do this with a constexpr function either...

With this in mind, is it really worthwhile to pursue it?

In the meantime, is the patch as it stands good enough to commit?!

Cheers
Andy



-------------- next part --------------
A non-text attachment was scrubbed...
Name: strlen.diff
Type: application/octet-stream
Size: 9692 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120623/28f1c973/attachment.obj>