[PATCH] D37035: Implement __builtin_LINE() et. al. to support source location capture.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 10 inline comments as done.
EricWF added inline comments.


================
Comment at: include/clang/AST/SourceLocExprScope.h:39
+    //    static_assert(foo() == __LINE__);
+    return OldVal == nullptr || isa<CXXDefaultInitExpr>(DefaultExpr) ||
+           !OldVal->isInSameContext(EvalContext);
----------------
rsmith wrote:
> Do we want the `isa` check here? I'd think the same problem would occur for default initializers:
> 
> ```
> struct A {
>   int n = __builtin_LINE();
> };
> struct B {
>   A a = {};
> };
> B b = {}; // should give this context as the current one, not the context of the initializer inside struct B
> ```
Good catch. Fixed.


================
Comment at: include/clang/AST/SourceLocExprScope.h:44
+public:
+  SourceLocExprScopeBase(const Expr *DefaultExpr, SourceLocExprScopeBase **Dest,
+                         EvalContextType *EvalContext)
----------------
rsmith wrote:
> I think it'd be cleaner to add a
> 
> ```
> struct Current {
>   SourceLocExprScopeBase *Scope;
> };
> ```
> 
> maybe with members to get the location and context for a given `SourceLocExpr`, and to pass in a `SourceLocExprScopeBase::Current &` here rather than a `SourceLocExprScopeBase **`.
Done. Hopefully I understood the comment.


================
Comment at: include/clang/Sema/Sema.h:4463
+
+  /// \brief build a potentially resolved SourceLocExpr.
+  ///
----------------
rsmith wrote:
> Capitalize "build" here.
Done and removed `\brief`.


================
Comment at: lib/AST/Expr.cpp:1924-1931
+  auto CreateString = [&](StringRef SVal) {
+    QualType StrTy = BuildSourceLocExprType(Ctx, getIdentType(),
+                                            /*IsArray*/ true, SVal.size() + 1);
+    StringLiteral *Lit = StringLiteral::Create(Ctx, SVal, StringLiteral::Ascii,
+                                               /*Pascal*/ false, StrTy, Loc);
+    assert(Lit && "should not be null");
+    return Lit;
----------------
rsmith wrote:
> It doesn't seem reasonable to create a new `StringLiteral` each time a `SourceLocExpr` is (constant-)evaluated, and seems like it would be unsound in some cases (we require that reevaluation produces the same result).
> 
> I think the `StringLiteral` here is just for convenience, right? (Instead we could model the `SourceLocExpr` as being its own kind of array lvalue for the purpose of constant expression evaluation, like we do for `ObjCEncodeExpr` for instance.)
OK. I think I have a solution for this. Please forgive my poor explanation of the problem. The problem was two fold:

First, the type of `SourceLocExpr` represents the decayed type of the underlying array, and not the array type or value-category itself (as `StringLiteral` and `ObjCEncodeExpr` do). Therefore it takes additional work to propagate and ensure that the existing machinery uses the "real array type" of the `SourceLocExpr` and not the type of the `SourceLocExpr` itself.

Second,,we need to determine, store, and propagate  the type,, value category, and value of a `SourceLocExpr` corresponding to the context in which we first encounter the  `SourceLocExpr. None of this contextual information is available in the AST after the fact. A lazy model where `SourceLocExpr` pretends to be a placeholder for the "real array type and value" until said subelements are actually accessed will produce different results than if the `SourceLocExpr` was correctly evaluated in the context it was initially encountered.

For Example:
```
const char *cur_file(const char *f = __builtin_FILE()) { return f; }
struct TestInit {
#line 100 "initial_scope.cpp"
  const char *my_file = cur_file();
  TestInit() {} // FILE should point here. We need to transform/determine the value and type of the `SourceLocExpr` in this context.
};
#line 200 "new_scope.cpp"
// If we delay evaluating `__builtin_FILE()` until this point, where it's actually needed, it will return `new_scope.cpp`,
// because we've already exited the `CXXDefaultInitExpr` scope created when first evaluating `TestInit() = default`. 
TestInit Default;
static_assert(strcmp(Default.my_file, "initial_scope.cpp") == 0); // Fails if lazily evaluated!
```

I worked around this problem by storing the "real array type", the used location, and the used context of  the `__builtin_FOO())` expression in the `LValueBase` produced when we first consider the `__builtin_FOO()` expression. 


================
Comment at: lib/AST/ExprConstant.cpp:698-704
+    /// \brief Source location information about the default argument expression
+    /// we're evaluating, if any.
+    SourceLocExprScope *CurCXXDefaultArgScope = nullptr;
+
+    /// \brief Source location information about the default member initializer
+    /// we're evaluating, if any.
+    SourceLocExprScope *CurCXXDefaultInitScope = nullptr;
----------------
rsmith wrote:
> Do we really need both of these? It would seem like one 'current' scope would be sufficient to me.
Indeed, we don't seem to need both so I've removed one.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:595
+    } else {
+      StringLiteral *Str = SLE->getStringValue(Ctx, Loc, DC);
+      return CGF.EmitArrayToPointerDecay(Str).getPointer();
----------------
rsmith wrote:
> Should we make some attempt to reuse the same string literal for multiple source locations denoting the same file?
I think I've addressed this. Instead of creating a `StringLiteral` expressions to evaluate, the new code uses `GetAddrOfConstentCString` to create the constant address corresponding to the string value specified by the `SourceLocExpr`.


https://reviews.llvm.org/D37035