This is an archive of the discontinued LLVM Phabricator instance.

Implement __builtin_LINE() et. al. to support source location capture.
ClosedPublic

Authored by EricWF on Aug 22 2017, 4:13 PM.

Details

Summary

This patch implements the source location builtins __builtin_LINE(), builtin_FUNCTION(), builtin_FILE() and __builtin_COLUMN(). These builtins are needed to implement [std::experimental::source_location`](https://rawgit.com/cplusplus/fundamentals-ts/v2/main.html#reflection.src_loc.creation).

With the exception of __builtin_COLUMN, GCC also implements these builtins, and Clangs behavior is intended to match as closely as possible.

Diff Detail

Event Timeline

EricWF created this revision.Aug 22 2017, 4:13 PM
hfinkel added inline comments.
include/clang/AST/Expr.h
3814

BuiltinSourceLocExpr -> SourceLocExpr

lib/Sema/SemaExpr.cpp
12955

Do you have an example where you don't get back a valid location? Are you going to give back ("", 0, 0) for (file, line, column)? Probably doesn't matter for the function name.

lib/Sema/SemaInit.cpp
563 ↗(On Diff #112249)
// Nothing to do.
majnemer added inline comments.
include/clang/AST/Expr.h
3816

BUILTIN_FILE -> builtin_FILE

3838

StringRef?

rsmith added inline comments.Aug 22 2017, 5:05 PM
test/SemaCXX/source_location.cpp
11–36

This seems suboptimal. It would seem better for the compiler to generate a global containing the relevant data and to represent a source_location as a pointer to such a value. We should also try to minimize the number of relocations necessary to build a source_location object, since such constructions are likely to be extremely common in some codebases. We should also keep in mind that we're likely to want to add fields to source_location in future, so designing it in a way that avoids an ABI break for such cases would be preferable.

How long has GCC supported this?

EricWF updated this revision to Diff 112268.Aug 22 2017, 5:28 PM
EricWF marked 4 inline comments as done.
  • Address inline comments.
  • Fix calls to class call operators.
rsmith edited edge metadata.Aug 22 2017, 5:35 PM

I don't like the model of conditionally rebuilding the default initializer / default argument if it contains one of these builtins; it seems more heavy-handed than necessary. I'm also not convinced that we need to store the computed value in the AST.

As an alternative, CodeGen and the expression evaluator can track a current call context (the innermost call expression or list initialization that is not within a default argument / default member initializer) consisting of the enclosing FunctionDecl and SourceLocation, and when the value of one of these builtins is needed, pass that context to the AST node to ask what value it has in that context. This will require a bit more work for the static analyzer, but should be fairly simple elsewhere and will avoid the extra AST nodes for the desugared form.

docs/LanguageExtensions.rst
2118

What is an NSDMI? I think you mean "default member initializer".

EricWF marked an inline comment as done.Aug 22 2017, 5:54 PM

I don't like the model of conditionally rebuilding the default initializer / default argument if it contains one of these builtins; it seems more heavy-handed than necessary. I'm also not convinced that we need to store the computed value in the AST.

As an alternative, CodeGen and the expression evaluator can track a current call context (the innermost call expression or list initialization that is not within a default argument / default member initializer) consisting of the enclosing FunctionDecl and SourceLocation, and when the value of one of these builtins is needed, pass that context to the AST node to ask what value it has in that context. This will require a bit more work for the static analyzer, but should be fairly simple elsewhere and will avoid the extra AST nodes for the desugared form.

That sounds good to me. Could you suggest where said context should correctly live?

docs/LanguageExtensions.rst
2118

"Non-static default member initializer". I'll change it to your suggestion.

include/clang/AST/Expr.h
3838

Sure. Why not. I'm never sure how to handle string literals.

alexr added a subscriber: alexr.Aug 24 2017, 8:50 AM
EricWF updated this revision to Diff 115161.Sep 13 2017, 8:35 PM
EricWF marked an inline comment as done.
  • Reimplement without rewriting the AST and instead during the substitution during constant expression evaluation and code gen.

I still haven't implemented Richards suggestion to reduce the size of source_location struct, but I think that would be better done as a separate patch. These builtins still need to be implemented, and to act like their GNU counterparts.

I haven't cleaned this patch up yet, but I plan to do so tomorrow. There are going to be silly mistakes until then.

EricWF added inline comments.Sep 13 2017, 8:40 PM
test/SemaCXX/loc2.cpp
1 ↗(On Diff #115161)

Didn't mean to include this file.

test/SemaCXX/test.cpp
1 ↗(On Diff #115161)

Didn't mean to include this file either.

EricWF added inline comments.Sep 13 2017, 8:50 PM
lib/AST/Expr.cpp
1940

Urg... Not sure how both clang-format and git -w messed up enough to let these whitespace changes through (and the ones below this).

lib/AST/ExprConstant.cpp
591

These members need documentation.

6279

This change seems unnecessary.

EricWF updated this revision to Diff 115162.Sep 13 2017, 9:07 PM
  • Remove accidentally committed test files.
  • Attempt to remove incidental whitespace changes.
EricWF added inline comments.Sep 14 2017, 3:10 AM
docs/LanguageExtensions.rst
2118

FIXME: This texts sucks. I can do better.

include/clang/AST/Decl.h
2418 ↗(On Diff #115162)

git clang-format strikes again...

lib/AST/Expr.cpp
1895

Remove default case to make way for diagnostics when new cases are added.

lib/AST/ExprConstant.cpp
4567

E->getUsedLocation()?

7134

This special control flow should be tested. I think it might be? But not against a NSDMI.

lib/CodeGen/CodeGenFunction.h
1250

redundant public.

1253

These need documentation.

1259

Needs documentation.

1339

Needs documentation.

EricWF updated this revision to Diff 115342.Sep 14 2017, 7:20 PM
EricWF marked 8 inline comments as done.
  • Cleanup and address the inline comments I added earlier.
EricWF updated this revision to Diff 115349.Sep 14 2017, 7:47 PM
  • Improve test.
EricWF updated this revision to Diff 129746.Jan 12 2018, 6:56 PM
  • Introduce SourceLocExprScope.h to help reduce code duplication.
  • Merge with upstream.
EricWF updated this revision to Diff 133294.Feb 7 2018, 1:31 PM
EricWF added reviewers: aaron.ballman, bogner, majnemer.

Ping.

  • Rebase off trunk.
EricWF updated this revision to Diff 149007.May 29 2018, 5:18 PM
  • Merge with upstream.

Ping.

EricWF updated this revision to Diff 149009.May 29 2018, 5:31 PM
  • Fix merge conflicts.

Any news on the status of this ? Would be really nice to have it in.

EricWF marked 5 inline comments as done.May 30 2018, 10:24 AM

Any news on the status of this ? Would be really nice to have it in.

I think this patch is ready to go; but there is more work to be done, as a separate patch, to support std::source_location in a better manner as suggested by @rsmith in the inline comments.

test/SemaCXX/source_location.cpp
11–36

@rsmith, I agree with your design for std::source_location. However, would it be acceptable for now to commit this patch as only the builtins, since we need those for GCC compatibility anyway?

Then the changes for something like __builtin_source_location() can be added afterwards?

rsmith added inline comments.Jun 15 2018, 5:48 PM
include/clang/AST/SourceLocExprScope.h
39 ↗(On Diff #149009)

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
44 ↗(On Diff #149009)

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

include/clang/Sema/Sema.h
4463

Capitalize "build" here.

include/clang/Serialization/ASTBitCodes.h
1643

Missing period.

lib/AST/Expr.cpp
1924–1931

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

lib/AST/ExprConstant.cpp
698–704

Do we really need both of these? It would seem like one 'current' scope would be sufficient to me.

7296

Too much indentation.

lib/CodeGen/CGExprAgg.cpp
192–202

Something funny happened to the formatting here.

lib/CodeGen/CGExprScalar.cpp
605

Should we make some attempt to reuse the same string literal for multiple source locations denoting the same file?

EricWF marked 10 inline comments as done.Jun 27 2018, 2:21 AM
EricWF added inline comments.
include/clang/AST/SourceLocExprScope.h
39 ↗(On Diff #149009)

Good catch. Fixed.

44 ↗(On Diff #149009)

Done. Hopefully I understood the comment.

include/clang/Sema/Sema.h
4463

Done and removed \brief.

lib/AST/Expr.cpp
1924–1931

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.

lib/AST/ExprConstant.cpp
698–704

Indeed, we don't seem to need both so I've removed one.

lib/CodeGen/CGExprScalar.cpp
605

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.

EricWF updated this revision to Diff 153024.Jun 27 2018, 2:27 AM
EricWF marked 6 inline comments as done.

Address most review comments:

  • No longer create StringLiteral temporaries during both constant evaluation and code generation.
  • Attempt to clean up the SourceLocExprScopeGuard interface as requested.

Additionally add a new test CodeGenCXX/builtin-source-location.cpp to better ensure that evaluation during CodeGen acts the same as constant evaluation. The test is still a WIP.

EricWF marked 5 inline comments as done.Jun 27 2018, 2:53 AM
EricWF planned changes to this revision.Jun 30 2018, 1:46 AM

Previously this patch performed and leaked a lot of StringLiteral temporaries.

I think I have a better solution for that problem, but this patch isn't it.

Updating patch incoming.

EricWF updated this revision to Diff 153840.Jul 2 2018, 6:44 PM

When evaluating a SourceLocExpr returning a string literal, we have to produce the string within the context the SourceLocExpr appears and then propagate it within the resulting LValue. We cannot lazily produce the value when a character of the string is actually accessed.

This change adds the field const char *LValueBase::LValueString to allow storing and propagating the result of evaluating a SourceLocExpr, as well as providing the required information to compute the "true" ConstantArrayType for an LValueBase which refers to a SourceLocExpr.

@rsmith How does this approach look to you?

This prevents the need to produce new StringLiteral expressions. However I'm not sure this is the best approach, since it increases the complexity when accessing an LValueBase in ExprConstant.cpp, in particular producing the type of the LValueBase (which has to be invented on the fly).

An alternative solution would be to create a StringLiteral, cache it in ASTContext for reuse, and return the new expression. Would this require serializing the ASTContext cache? Would this be a better solution?

EricWF updated this revision to Diff 153856.Jul 2 2018, 10:25 PM
  • Store the ConstantArrayType in LValueBase so ExprConstant doesn't have to repeatedly recompute it.
EricWF updated this revision to Diff 153957.Jul 3 2018, 12:31 PM
  • Add polish.

I think this is good to go.

EricWF updated this revision to Diff 153959.Jul 3 2018, 12:34 PM
  • Fix test regex.

This is in good shape. Some relatively minor comments (plus there are some unrelated whitespace changes in a few files); the only nontrivial comment I have is that I think it'd be cleaner to cache StringLiteral*s rather than adding a new form of LValueBase for the case of a global constant string with no associataed StringLiteral.

docs/LanguageExtensions.rst
2179

rST uses double-backticks for code font. Does this URL get autolinked?

rsmith added inline comments.Aug 30 2018, 6:46 PM
include/clang/AST/APValue.h
65–72

This seems like a slightly dangerous overload set (eg, what does LValueBase(P, 0, 0); call when P is of type Expr*?)

114–119

This makes LValueBase 8 bytes larger (on 64-bit targets) to support an uncommon case. Does APValue get larger too, or is its size dominated by something else? If needed, we could avoid the size increase by separately allocating the GlobalStringData and storing a pointer to it here -- and maybe something like a GlobalStringData* is what we should be caching on the ASTContext instead of a const char*?

However, as you mentioned in a prior comment:

An alternative solution would be to create a StringLiteral, cache it in ASTContext for reuse, and return the new expression. Would this require serializing the ASTContext cache? Would this be a better solution?

Yes, I think that'd be a better and cleaner solution. We shouldn't need to serialize the ASTContext cache, because we never serialize APValues. (And if we ever start doing so, serialization of lvalues denoting Expr*s becomes a hard problem in general, not only in this case.)

Naturally, you'll need to cache the strings used by __builtin_FILE in addition to the current cache for strings used by __builtin_FUNCTION, but on the plus side you should be able to reuse the existing code that generates StringLiteral objects for PredefinedExprs (and maybe you could even share a cache between the new builtins and the PredefinedExpr handling?)

lib/AST/APValue.cpp
46–49

This seems to be unused?

lib/AST/ASTContext.cpp
9915

Please use PredefinedExpr::ComputeName here to reuse the same logic that we use for __FUNCTION__. We don't want __FUNCTION__ and __builtin_FUNCTION() to return different strings for the same function.

lib/AST/ExprConstant.cpp
72

Why do you need to desugar the type here? getType can generally return a sugared type.

3353

Similarly, why do you need to desugar the type here?

lib/CodeGen/CGExprConstant.cpp
867–869

Why was this removed?

868–869

I think the salient point here is: this visitor will fail if it reaches a SourceLocExpr, so there's no need to track state that only it needs. (Same reason we didn't need a CXXDefaultInitExprScope for handling CXXThisExpr.)

1444–1446

Preferably commit this minor formatting cleanup separately.

lib/CodeGen/CGExprScalar.cpp
592

Please comment in the form of a sentence :)

EricWF updated this revision to Diff 166622.Sep 22 2018, 6:27 PM
EricWF marked 8 inline comments as done.
  • Merge with upstream.
  • Address some of the easier review comments.
EricWF added inline comments.Sep 30 2018, 11:11 AM
docs/LanguageExtensions.rst
2179

Woops. Fixed.

lib/AST/APValue.cpp
46–49

It was used, but I folded the definition into that usage.

lib/AST/ExprConstant.cpp
3353

It does seem redundant. Removing.

I think I was using it to generate a QualType from a Type*.

lib/CodeGen/CGExprConstant.cpp
867–869

I believe it was dead code?

868–869

Ack. Removing the comment.

EricWF marked 10 inline comments as done.Sep 30 2018, 2:28 PM
EricWF added inline comments.
include/clang/AST/APValue.h
65–72

Removed in place of the string literal caching implementation.

114–119

Yes, I think that'd be a better and cleaner solution. We shouldn't need to serialize the ASTContext cache, because we never serialize APValues. (And if we ever start doing so, serialization of lvalues denoting Expr*s becomes a hard problem in general, not only in this case.)

SGTM. I've implemented this instead.

on the plus side you should be able to reuse the existing code that generates StringLiteral objects for PredefinedExprs (and maybe you could even share a cache between the new builtins and the PredefinedExpr handling?)

The only issue with caching, and in particular adding it to PredefinedExpr, is that the StringLiteral no longer has meaningful location information. Is this OK?

EricWF marked 3 inline comments as done.Sep 30 2018, 2:29 PM
EricWF updated this revision to Diff 167664.Sep 30 2018, 2:34 PM

Build, cache and substitute the SourceLocExpr for a StringLiteral during constant evaluation. This is instead of the changes to LValueBasewhich stored the string value and type in addition to the original SourceLocExpr.

Currently only SourceLocExpr uses the StringLiteral cache in ASTContext. PredefinedExpr could be made to use it at the expense of losing the SourceLocation information the StringLiteral caches.

@rsmith How does this look now?

I'll follow up with a separate __builtin_SOURCE_LOCATION() patch which returns a single relocatable object containing the file, func, line and column. @rsmith Any pointers on how to implement that?

EricWF updated this revision to Diff 179211.Dec 20 2018, 5:42 PM

Merge with upstream.

EricWF added a comment.Jan 5 2019, 5:42 PM

Ping! I would like to land this before the next release.

riccibruno added inline comments.
include/clang/AST/Expr.h
4108

IdentType -> IdentKind ?

4147

I don't think that LLVM_READONLY is useful here since it is just a simple getter.
Same remark for getIdentType, isStringType and isIntType.
And even for getBuiltinStr does it actually make a difference ?

include/clang/AST/Stmt.h
294

Could you please put this in the same place as in
StmtNodes.td ? (ie just after PseudoObjectExpr)
At least not under "C++ coroutines TS bitfields classes"...

lib/AST/ASTContext.cpp
10145

And what about C ? It seems to me that getStringLiteralArrayType
should be used systematically when the type of a string literal is needed.

(eg in ActOnStringLiteral but this is not the only place...)

lib/AST/Expr.cpp
1961

Is it taking into account adjustStringLiteralBaseType as in getStringLiteralArrayType ?

1978

The name Type here is really unfortunate imho.

1992

llvm_unreachable("unexpected IdentType!")

2025

This comment is strange since MkStr will call getPredefinedStringLiteralFromCache
when FD->getDeclName() is a simple identifier.

2037

Both getLine, getColumn and APInt take an unsigned.

EricWF marked 13 inline comments as done.Jan 7 2019, 1:25 PM

Address review comments. Updated patch coming shortly.

include/clang/AST/Expr.h
4147

It probably doesn't make any difference. Removing.

lib/AST/ASTContext.cpp
10145

It should be. I'll fix it to act like ActOnStringLiteral and then deduplicate the code.

lib/AST/Expr.cpp
1961

Nope. Fixed.

2025

I think this should say compute instead of cache.

2037

What's your objection here? That I used int64_t or getIntWidth()? I've reworked this bit of code. Let me know if it's still problematic.

rsmith added inline comments.Jan 7 2019, 4:22 PM
include/clang/AST/ASTContext.h
279

Comment seems out of date: the key here is a string rather than a function declaration.

lib/AST/Expr.cpp
2012–2020

Combining these two cases doesn't really make sense since they have nothing in common (except that both produce strings). Instead, consider hoisting the MkStr lambda out of the switch and separating the cases.

2022–2023

Why not call into PredefinedExpr for the other cases that __FUNCTION__ supports (BlockDecl, CapturedDecl, ObjCMethodDecl)?

2026–2027

Is the benefit of avoiding a std::string allocation here really worth the cost of duplicating this portion of the __FUNCTION__ logic?

If we care about the extra string allocation, it'd seem more worthwhile to extend ComputeName so that a stack-allocated SmallString<N> buffer can be passed in (eg, StringRef ComputeName(..., SmallVectorImpl<char> &NameStorage), where NameStorage may, but need not, be used to store the resulting string).

lib/AST/ExprConstant.cpp
2653–2655

Hm, why the move of the get from caller to callee? This widens the set of possible arguments to extractStringLiteralCharacter, but all the new possibilities result in crashes -- shouldn't the caller, rather than the callee, still be the place where we house the assertion that the base is indeed an expression?

3370–3371

There are lots of forms of expression that cannot be the base of an LValue (see the list above LValueExprEvaluator for the expression forms that *can* be the base of an LValue); is it important to give this one special treatment?

lib/CodeGen/CGExprConstant.cpp
1743

Hmm, is this reachable? I thought SourceLocExpr was always a prvalue.

1747

const auto *Str

1749–1750

Please remove or uncomment this.

lib/Sema/TreeTransform.h
9990

If we generalize __builtin_FUNCTION() to behave like __FUNCTION__ (and handle function-like contexts that aren't FunctionDecls), this isa check will be wrong.

EricWF marked 18 inline comments as done.Jan 7 2019, 6:02 PM

Mark more review comments as done.

lib/AST/Expr.cpp
2026–2027

Probably. not. I think this optimization attempt is a remnant from an older caching approach.

lib/AST/ExprConstant.cpp
2653–2655

Remant of an older version of the patch. I'll revert.

Sorry, I'll try being better about reviewing patches for unnecessary changes.

3370–3371

Because a SourceLocExpr *can* be the base of an LValue. But the way that's supported is by transforming the SourceLocExpr into a StringLiteral.

lib/CodeGen/CGExprConstant.cpp
1743

You're right. Not sure what I was thinking.

lib/Sema/TreeTransform.h
9990

I'll remove it.

EricWF updated this revision to Diff 180597.Jan 7 2019, 6:03 PM
EricWF marked 4 inline comments as done.

Address review comments.

riccibruno marked an inline comment as done.Jan 8 2019, 5:35 AM

I have no more remark, but since I am just a new contributor to clang I am sure
people will have more to say about this patch. Thanks !

lib/AST/Expr.cpp
2091

What's your objection here? That I used int64_t or getIntWidth()? I've reworked this bit of
code. Let me know if it's still problematic.

It was just a very small remark saying that you were doing unsigned -> signed -> unsigned
for no apparent reason. Anyway the current way looks fine to me.

EricWF marked 2 inline comments as done.Jan 14 2019, 11:44 AM
rsmith accepted this revision.Apr 25 2019, 3:57 PM

Some cleanups and simplifications, but LGTM with those applied. Thank you!

docs/LanguageExtensions.rst
2335

appears -> appear

include/clang/AST/CurrentSourceLocExprScope.h
32

gaurd -> guard

include/clang/AST/Expr.h
4168

Missing period at end of comment.

lib/AST/ASTContext.cpp
10151–10153

"the string length for pascal strings" part here seems out of place, since you don't handle that here.

lib/AST/Expr.cpp
2073

Given that the type of the SourceLocExpr is const char*, I'd expect the APValue result to be a decayed pointer to the first character in the string, not a pointer to the string itself. (So I think this should have a path of length 1 containing LValuePathEntry{.ArrayIndex = 0}.)

lib/AST/ExprConstant.cpp
3370–3371

I don't agree: a SourceLocExpr cannot be the base of an LValue. It is evaluated into something else that can be (a StringLiteral), but it itself cannot be (and this is in no way unusual; that's probably true of most Expr nodes). I think this is a remnant of an earlier design?

5861–5863

(This won't be necessary if/when EvaluateInContext returns a pointer to the first character in the string.)

7616

Consider dropping the .getInt() here? (Then this version and the pointer version will be identical, and we could consider moving this to ExprEvaluatorBase.)

11286

This comment is getting detached from its case label. Maybe just remove it? It seems reasonably obvious that __null should be an ICE.

lib/CodeGen/CGExpr.cpp
3241–3281

This change should be unnecessary after changing EvaluateInContext to decay the pointer.

lib/CodeGen/CGExprScalar.cpp
588–599

This can be simplified to:

return ConstantEmitter(CGF.CGM, CGF).emitAbstract(SLE->getLocation(), Evaluated, SLE->getType());

(once EvaluateInContext decays the pointer).

lib/Serialization/ASTReaderStmt.cpp
915–921

Please directly set the fields here and remove the setters; we generally don't want AST nodes to have setter functions, as they're supposed to be immutable after creation. (Deserialization is an exception to this only because it is creating the nodes itself.)

This revision is now accepted and ready to land.Apr 25 2019, 3:57 PM
EricWF marked 14 inline comments as done.May 16 2019, 1:43 PM

Address review comments.

lib/AST/ExprConstant.cpp
3370–3371

Seems like it. Removed.

EricWF updated this revision to Diff 199893.May 16 2019, 1:47 PM
EricWF marked an inline comment as done.

Merge with upstream.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2019, 2:01 PM
Herald added a subscriber: kristina. · View Herald Transcript

Looks like this test is failing on SystemZ since it was added, making all our build bots red:

/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/tools/clang/test/CodeGenCXX/builtin_FUNCTION.cpp:9:11: error: CHECK: expected string not found in input
// CHECK: @[[EMPTY_STR:.+]] = private unnamed_addr constant [1 x i8] zeroinitializer, align 1
          ^
/home/uweigand/sandbox/buildbot/clang-s390x-linux/stage1/tools/clang/test/CodeGenCXX/Output/builtin_FUNCTION.cpp.tmp.ll:1:1: note: scanning from here
; ModuleID = '/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/tools/clang/test/CodeGenCXX/builtin_FUNCTION.cpp'
^
/home/uweigand/sandbox/buildbot/clang-s390x-linux/stage1/tools/clang/test/CodeGenCXX/Output/builtin_FUNCTION.cpp.tmp.ll:8:1: note: possible intended match here
@.str = private unnamed_addr constant [1 x i8] zeroinitializer, align 2
^

The problem is that string constants are 2-aligned according to the SystemZ ABI (this is a bit different, but deliberate in order to allow computing their addresses with a LARL instruction). This makes all the "align 1" checks fail.

Is this test deliberately verifying the alignment, or could this just be removed?

As of r361920, the SystemZ build bots are green again. Thanks, Eric!