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

[Richard Smith]

On Fri, Jun 22, 2012 at 3:46 PM, Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:
> 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.

This will be because you're still using Expr::EvaluateAsRValue: that
won't work for references to constexpr function parameters, because it
doesn't operate in the same EvalInfo context.

> 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.

This approach has the downside of copying the string even when it's a
string literal. This construct appears pretty frequently in code using
various glibc functions which are implemented as macros, so that may
be a significant issue for some code.

> I kept with EvaluateAsRValue,
> since this works adequately and, I'm afraid to say, I couldn't get
> EvaluatePointer to work at all.

What sort of issues were you having?

[...]
> 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);

We don't allow this for a constexpr function, so we don't need to
allow it here either. Indeed, there are simple cases where this would
be impossible:

struct X { char c[8]; void *p; };
constexpr X x = { 1, 2, 3, 4, 5, 6, 7, 8, &x; };
constexpr int n = __builtin_strlen(x.c); // ???

> 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?

No, I don't think so.