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

[Andy Gibbs]

On Saturday, June 23, 2012 1:34 AM, Richard Smith wrote:
> On Fri, Jun 22, 2012 at 3:46 PM, Andy Gibbs <andyg1001 at hotmail.co.uk> 
> wrote:
>> 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.

Ok, I didn't realise this was the reason -- I'm still learning how the code 
works!

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

Yes, this is a fair point and it is the unfortunate side-effect of my 
attempt to make what I saw as a useful feature out of the code.  I have an 
alternative suggestion, please see below.

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

There is an assert at the top of EvaluatePointer to ensure Expr is an 
rvalue, which is not necessarily the case.  I'm afraid I don't have the code 
nor my test-cases in front of me right now so I can't give you an exact 
example.  Since I wasn't aware (as I said above) of the disadvantages of 
using EvaluateAsRValue, and because I had found in my testing that using 
EvaluatePointer actually did not work in any case in the function argument 
case, I kept with EvaluateAsRValue since it worked in all other scenarios. 
Hence my thought (erroneous, from what you say) that this issue with 
function arguments was due to some other reason.

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

Good, but you had me concerned by including it in your corner cases.

So, now for my suggestion.  Actually, I have two.  My first is this: as one 
coming from the outside, ExprConstant.cpp is quite a difficult block of code 
to follow, since a lot of different functionality is contained and 
interwoven.  My suggestion would be to separate out ExprConstant.cpp into 
separate files in an ExprConstant subfolder.  It could then be possible to 
have the Expr member functions in one file, general purpose utility 
functions in another (e.g. the IsGlobalLValue and similar), and then the 
Visitor classes then split out into their own .h/.cpp files, and so on. 
This approach would make it much easier for others to learn the intricacies 
of this particular part of the compiler.  Now, I would be quite willing to 
undertake this work, *if* (and its a big if) I could feel that if I did the 
work, that it would be appreciated, and more importantly, committed.  So 
please say.  I don't mind either way, but I don't want to undertake it for 
it to be simply rejected.

My suggestion, then, for an alternative to my previous patch would be to 
implement a string length evaluation visitor class (sorry, I haven't thought 
up a groovy class name for it yet!).  This would then visit the Expr object, 
calculating any pointer arithmetic operations as it goes, down to either a 
StringLiteral expression, or a DeclRefExpr pointing to a StringLiteral 
variable or constexpr char array.  The length can be  calculated direct from 
this and the precalculated offset.  No unnecessary string copying or LValue 
generation!  I should expect this is the most efficient way, better even 
than EvaluatePointer and all the HandleLValueToRValueConversion operations 
as per your previous suggestion?

As you know, I am still learning my way around the code; I really do 
appreciate your time in all your advice and corrections.

Please let me know what you think of my two suggestions.  I stand by ready 
and willing...

Thanks again,
Andy