Page MenuHomePhabricator

C99 partial re-initialization behavior (DR-253)
ClosedPublic

Authored by ygao on Oct 14 2014, 6:09 PM.

Details

Reviewers
rsmith
Summary

Hi all,

Based on previous discussion on the mailing list, clang currently lacks support
for C99 partial re-initialization behavior:
Reference: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2013-April/029188.html
Reference: http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_253.htm

I am proposing the following patch hopefully to fix this problem.

Given the following code snippet,

struct P1 { char x[6]; };
struct LP1 { struct P1 p1; };

struct LP1 l = { .p1 = { "foo" }, .p1.x[2] = 'x' };
// this example is adapted from the example for "struct fred x[]" in DR-253;
// currently clang produces in l: { "\0\0x" },
//   whereas gcc 4.8 produces { "fox" };
// with this fix, clang will also produce: { "fox" };

The idea is that upon seeing the second initializer, we will add a member to
the InitListExpr corresponding to ".p1" to hold the expression "{ "foo" }",
whereas one child field of the InitListExpr will hold "x". We keep this
structure till the CodeGen phase where we evaluate "{ "foo" }" and recursively
pass down its (evaluated) values as needed to the child fields of ".p1". The
recursion will stop at the child fields which are initialized with a
brace-enclosed list (if they exist).

From the Sema phase till the codeGen phase, the initializers for "l.p1" are
stored in two separate places: the new PrevInitExpr member of the top-level
InitListExpr, and the various child elements of the InitListExpr. This makes
it more complicated to do synatic/semantic checking of the initializer list.
This proposed patch does not cover all the semantic checking cases, but a
subsequent patch might be needed to do all the semantic checking on the new
PrevInitExpr member.

In the case where a string literal appears in place of "{ "foo" }", clang
evaluates it to a llvm::ConstantDataArray, which is not very easy to pass down
its components down to the child fields. In this proposal, I implemented a
method to build a llvm::ConstantArray out of a given ConstantDataArray. This is
the only part of the patch implemented in the llvm backend and will need to be
committed before the other parts. Or, maybe there are better ways to solve this?

Could someone take a look at the patch?

  • Gao

Diff Detail

Event Timeline

ygao updated this revision to Diff 14906.Oct 14 2014, 6:09 PM
ygao retitled this revision from to C99 partial re-initialization behavior (DR-253).
ygao updated this object.
ygao edited the test plan for this revision. (Show Details)
ygao added a reviewer: rsmith.
ygao added subscribers: Unknown Object (MLST), Unknown Object (MLST).
rsmith requested changes to this revision.Oct 20 2014, 4:57 PM
rsmith edited edge metadata.

The right way to deal with this is to expand initializers like { "foo" } to { 'f', 'o', 'o' } in the structured form of the InitListExpr if a designator tries to replace an element of the list.

llvm/tools/clang/include/clang/AST/Expr.h
3784 ↗(On Diff #14906)

This is a huge and unacceptable layering violation. It is not OK for the AST to refer to IR-level constructs.

This revision now requires changes to proceed.Oct 20 2014, 4:57 PM
asl added a subscriber: asl.Oct 21 2014, 4:38 AM
ygao updated this revision to Diff 15900.Nov 6 2014, 4:44 PM
ygao edited edge metadata.

Hi Richard,
A lot of thanks for your feedback. I revised the patch based on your feedback.
More advice would be appreciated,

  • Gao
ygao added a comment.Nov 11 2014, 2:18 PM

A gentle ping.

ygao updated this revision to Diff 16593.Nov 24 2014, 6:25 PM
ygao edited edge metadata.

Ping.
Re-based patch to trunk r222712.

ygao added a comment.Dec 2 2014, 10:46 AM

A post-Thanksgiving ping.

ygao added a comment.Dec 10 2014, 11:24 AM

Another ping.

ygao added a comment.Dec 16 2014, 1:53 PM

Yet another ping

ygao added a comment.Dec 24 2014, 4:30 PM

ping.
Happy Christmas!

Commented on a test. The functional change is way out of my league.
Also consider adding a "three-pass" test case similar to

struct B { int b1; int c; int b2; };
struct A { int a1; struct B b; int a2; };
struct A a = { 1, { 2, 3, 4 }, 5,
             .b = { 6, 7, 8 },
                .b.c = 9
};
struct A expected = { 1, { 6, 9, 8 }, 5 };
assert(!memcmp(a, expected));
test/CodeGen/partial-reinitialization1.c
55–62

You don't need any of these lines, right?
(If you ran this test on triples other than x64, you might want something like this, but it would have to be in lieu of the CHECK on line 67, not just defining an unused macro up here.)

Hi Gao,
I just noticed you had llvm-commits on the cc list, but as this is a
clang-only patch it seems that would be unnecessary. And it might cause
email to be mis-filed, for example I have mine set up to put all email
sent to both llvm-commits and cfe-commits to ignore the cfe-commits
recipient.
--paulr

ygao removed a subscriber: Unknown Object (MLST).Jan 7 2015, 12:52 PM
ygao updated this revision to Diff 17871.Jan 7 2015, 12:56 PM

Hey Arthur,
You are right about the test file. I do not need the macro definitions. I think I copied those lines
from another test. I added the three-pass test you suggested.
Thanks for reviewing the patch!

I also removed llvm-commits from the subscriber list,

Sorry for the delay; I've been thinking about this problem for a while and I think I now have a reasonable design for it. Consider a case like this:

struct Q { int a, b, c; };
Q *f();
void g() {
  struct A { Q q; } a = { *f(), .q.b = 3 };
}

Here, we can't model the initialization of a.q as a partial merge (because we have no way to represent initializing a.q.a and a.q.c from the same call to *f(), but initializing a.q.b from 3). I don't like the design of extending InitListExpr with a PrevInitExpr because it makes all users of that type need to cope with a weird special case; I'd prefer a more uniform representation.

So here's what I propose: add a DesignatedInitUpdateExpr that represents updating an initializer with another initializer by way of a designated initializer. This would have three members: the "base" initializer, the "update" initializer representing an initializer for a subobject of the "base", and a list of designators referring to the field of the base that is updated.

The evaluation semantics for a DIUE would be to first initialize the object using the "base" initializer, and then re-initialize the designated subobject using the "update" initializer. If the updated subobject is of a type that needs destruction, we'll need to pick between either running the destructor for the updated subobject, or rejecting designated initializations that would re-initialize a subobject with a non-trivial destructor (we may also need to take care if the updated subobject is a union member, because the initialized subobject might be a different one).

So in the above case, the initializer for a would be:

  • An InitListExpr for a, with type A
  • The init for field 0 (q) is a DesignatedInitUpdateExpr, with type Q and designator b
  • The "base" for the DIUE is the expression *f()
  • The "update" for the DIUE is the expression 3

We could still use something like your partialMergeFrom functionality where possible, and only create DesignatedInitUpdateExprs when necessary, but there are implications here: if an initializer has side-effects DIUE can preserve those side-effects even if the initializer is completely overridden, whereas partialMergeFrom will always lose the side-effects. The C standard allows either choice here, but I think we should be as consistent as we reasonably can. I suggest that we use partialMergeFrom wherever possible, and put the DIUE at the 'lowest' place possible within the initializer, so that we (attempt to) discard side-effects from all expressions that don't contribute to the final result. If we discard a side-effecting initializer in this way, we should issue a warning.

One other thing: partialMergeFrom should be a static function in SemaInit.cpp, not a member of InitListExpr (and maybe it might make more sense to integrate its functionality directly into InitListChecker). It's too high-level to belong on the AST.

ygao updated this revision to Diff 21546.Mar 9 2015, 10:48 PM

Hi Richard,
Here is an updated patch based on your suggestion earlier. We discussed this
briefly during the social, but for the record, I tried several different representations
for the vector of "updater" expressions and settled on using an InitListExpr for now.
Using an InitListExpr avoids having to decode a DesignatedInitExpr in
the CodeGen phase, and also allows merging multiple DesignatedInitExpr into
a common data structure, which then allows better diagnostics. The problem of
having to leave "holes" in an InitListExpr (to account for fields that are initialized by
the "base" expression and not to be overwritten by zeros) is solved by inserting a
new type of NoInitExpr as placeholders into these "holes."

There are two new functions in lib/AST/StmtProfile.cpp which I had to add in
order to build clang, but for which I am having trouble writing coverage tests. I
left a FIXME note there for now.

Feedback and advice are greatly appreciated,

  • Gao
ygao updated this revision to Diff 22530.Mar 23 2015, 4:20 PM

Updating the patch to trunk r232975.
A very gentle ping?

ygao added a comment.Apr 1 2015, 12:54 PM

A gentle ping.

ygao added a comment.Apr 14 2015, 10:59 AM

A more real ping.

ygao added a comment.Apr 23 2015, 11:56 AM

I ping, therefore I am.

davide added a subscriber: davide.Apr 27 2015, 6:20 PM

This is looking really good. I'm not entirely sure that NoInitExpr is necessary -- we could define a null Expr* within the update list of a DesignatedInitUpdateExpr as meaning "perform no initialization" instead -- but we can look at removing it after this patch lands.

include/clang/AST/Expr.h
4313

Put the * on the right, please, and capitalize as lBraceLoc / rBraceLoc.

lib/AST/Expr.cpp
2779

I would think this should be:

return Base->isConstantInitializer(...) && Update->isConstantInitializer(...)
3999

Given that we store the brace locations on the Updater, do we need to also store them inside DesignatedInitUpdateExpr?

4004–4014

This will give a range of Base start to Updater end if there's no LBraceLoc / RBraceLoc, which is a source range that can contain things unrelated to the update. Updater's end loc is always the same as RBraceLoc anyway, so should this instead return getBase()->getLocEnd()?

lib/AST/StmtProfile.cpp
716–724

To test these, put a DesignatedInitUpdateExpr in the signature of a function template, and declare it twice:

template<typename T> void f(decltype(T((char[4]){"foo", [2] = 'x'}))) {} // expected-note {{previous}}
template<typename T> void f(decltype(T((char[4]){"foo", [2] = 'r'}))) {} // ok
template<typename T> void f(decltype(T((char[4]){"boo", [2] = 'x'}))) {} // ok
template<typename T> void f(decltype(T((char[4]){"foo", [2] = 'x'}))) {} // expected-error {{redefinition}}
lib/CodeGen/CGExprCXX.cpp
955–972

This seems like a good idea, but not obviously related to this change. Can you split this out into a separate patch?

lib/Sema/SemaInit.cpp
653–664

If we're using NoInit here, we shouldn't PerformEmptyInit, because that might fail and in any case will wastefully produce an AST node that we don't want.

2187–2204

I would prefer to see this case handled by teaching AST/ExprConstant to evaluate DesignatedInitUpdateExpr and always using the code below, rather than trying to convert an existing initializer into an InitListExpr.

2829–2831

Is it useful to warn on overriding an array filler with an explicit value? This case seems unlikely to be a bug.

test/CodeGen/partial-reinitialization2.c
3

Executable/interpreted tests aren't acceptable here. Please check for specific IR being generated instead.

ygao added a comment.Apr 30 2015, 3:58 PM

Thanks for reviewing this.

lib/AST/Expr.cpp
2779

This function may get called in the middle of the initialization process, at which point the updater still contains unfilled "holes". I do not really want to call isConstantInitialzier() on such an updater, therefore the assertion here. Hmm, since I am so confident that Base will not be a constant initializer, maybe I can even just set culprit to Base without running isConstantInitializer() on it?

3999

I think you are right. I can get the brace locations from the updater without storing a duplicate copy.

4004–4014

Updater's end loc is always the same as RBraceLoc anyway.

Are you sure? If the RBraceLoc is not valid, then the updater's RBraceLoc is not valid either, so updater->getLocEnd() should return the end location of the last non-null initializer.

rsmith added inline comments.Apr 30 2015, 4:08 PM
lib/AST/Expr.cpp
2779

I don't think we should be calling this during initialization at all. If we really want to pull apart, say, a StringLiteral into an InitListExpr of CharacterLiterals (and I'm not convinced that we do, for source fidelity reasons), we should just do that directly in SemaInit without regard to whether the initializer supports constant emission.

4004–4014

OK, good point. Nonetheless, the range from getLocStart() to getLocEnd() should cover a contiguous range of code covering only this expression, and if the brace locations are invalid, then we'll use Base->getLocStart() to Updater->getLocEnd(), which might cover other stuff.

ygao added inline comments.May 4 2015, 12:30 PM
lib/AST/StmtProfile.cpp
716–724

Looks like StmtProfiler::VisitInitListExpr() only works on the syntactic form, and the syntac form
does not contain any DesignatedInitUpdateExpr. I can leave these two methods unimplemented.
I will add your test cases.

lib/CodeGen/CGExprCXX.cpp
952–953

There is a problem here. If I make a clean checkout newer than r234097 (I used r236298) and
comment out these three lines above, clang would crash with the test case below, even though
they are supposed to be just an optimization. r234095 is the last revision which works without
this assertion.

$ clang -S -std=c++11 -emit-llvm -o - test.cpp
Assertion `!PointeeType || PointeeType == getSourceElementType()' failed.

// test.cpp
int n;
struct T { int a; };
void *r = new T[n][3]{ { 1, 2, 3 }, { 4, 5, 6 } };

I am not sure if this is a real bug since I had to modify the compiler source, but If I make the rest of
the changes in this patch but leave out the new optimization below, I would be introducing the
same assertion. Maybe I can add this optimization first before the rest of the patch.

lib/Sema/SemaInit.cpp
2187–2204

You probably meant lib/CodeGen/CGExprConstant.cpp? I can move the logic of
partialMergeFrom() from Sema into CodeGen. The only down side seems to be that the
diagnostic becomes (arguably) less accurate. To illustrate, say we have the following two cases.

struct S {
  char L[6];
};

struct S
Case1[] = {
  "foo",          // expected-note{{previous initialization is here}}
  [0].L[4] = 'x'  // expected-warning{{subobject initialization overrides initialization of other fields}}
};

struct S
Case2[] = {
  { { 'f', 'o', 'o' } },
  [0].L[4] = 'x' // no-warning
};

If I break down a string literal into a tree of char literal initializers during semantic analysis, the compiler will treat the two cases the same and issue no warning for either. If I do not break down the string literal and delay till code gen, then in the Sema phase, the compiler only
sees that Case1[0].L in its entirety was initialized with a string literal, and so it will warn that Case1[0].L[4] is being reinitialized. It's a minor issue in my
opinion, but thought I should point it out during review.

ygao updated this revision to Diff 24911.May 4 2015, 1:12 PM

Updated the patch to address previous review comments.

ygao added a comment.May 12 2015, 7:01 PM

A gentle ping?

ygao added a subscriber: dblaikie.May 27 2015, 1:34 PM

Ping.

Also add David Blaikie. He might have some insight on whether lib/CodeGen/CGExprCXX.cpp#952-953 indicates any actual problem.
http://reviews.llvm.org/D5789?id=22530#inline-76127

ygao added a comment.May 27 2015, 3:45 PM

Ah, sorry I did not provide more context before pulling you in.

The assertion is related to GetElementPointer and I thought you may have worked on this recently;
also there was no assertion before your r234096+r234097. I am not sure if the assertion indicates
a real problem here because I had to modify the compiler source to show the problem. You probably
know better.

[Step#1] apply the following patch: it comments out an optimization.

Index: llvm/tools/clang/lib/CodeGen/CGExprCXX.cpp

  • llvm/tools/clang/lib/CodeGen/CGExprCXX.cpp (revision 238351)

+++ llvm/tools/clang/lib/CodeGen/CGExprCXX.cpp (working copy)
@@ -953,10 +953,12 @@

assert(getContext().hasSameUnqualifiedType(ElementType, Init->getType()) &&
       "got wrong type of element to initialize");

+#if 0

// If we have an empty initializer list, we can usually use memset.
if (auto *ILE = dyn_cast<InitListExpr>(Init))
  if (ILE->getNumInits() == 0 && TryMemsetInitialization())
    return;

+#endif

// Create the loop blocks.
llvm::BasicBlock *EntryBB = Builder.GetInsertBlock();

[Step#2] Compile the following test case:

// test.cpp
int n;
struct T { int a; };
void *r = new T[n][3]{ { 1, 2, 3 }, { 4, 5, 6 } };

$ build/Debug+Asserts/bin/clang -S -std=c++11 -emit-llvm -o - test.cpp
clang: llvm/include/llvm/IR/Instructions.h:842: static llvm::GetElementPtrInst* llvm::GetElementPtrInst::Create(llvm::Type*, llvm::Value*, llvm::ArrayRef<llvm::Value*>, const llvm::Twine&, llvm::Instruction*): Assertion `PointeeType == cast<PointerType>(Ptr->getType()->getScalarType())->getElementType()' failed.
0 clang 0x00000000046113de llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 44
1 clang 0x00000000046116f3
2 clang 0x00000000046101f4
3 libpthread.so.0 0x00007fedccedf340
4 libc.so.6 0x00007fedcc11bf79 gsignal + 57
5 libc.so.6 0x00007fedcc11f388 abort + 328
6 libc.so.6 0x00007fedcc114e36
7 libc.so.6 0x00007fedcc114ee2
8 clang 0x00000000017077ff
9 clang 0x00000000017078af
10 clang 0x0000000001764c8c
11 clang 0x00000000018f5662 clang::CodeGen::CodeGenFunction::EmitNewArrayInitializer(clang::CXXNewExpr const*, clang::QualType, llvm::Type*, llvm::Value*, llvm::Value*, llvm::Value*) + 3100
12 clang 0x00000000018f57a6
13 clang 0x00000000018f6d60 clang::CodeGen::CodeGenFunction::EmitCXXNewExpr(clang::CXXNewExpr const*) + 1950
14 clang 0x000000000190489c
15 clang 0x00000000019154af
16 clang 0x00000000019038d2
17 clang 0x00000000019095ba
18 clang 0x0000000001917257
19 clang 0x00000000019156d7
20 clang 0x00000000019038d2
21 clang 0x00000000019141b2 clang::CodeGen::CodeGenFunction::EmitScalarExpr(clang::Expr const*, bool) + 128
22 clang 0x00000000018b2e75 clang::CodeGen::CodeGenFunction::EmitScalarInit(clang::Expr const*, clang::ValueDecl const*, clang::CodeGen::LValue, bool) + 99
23 clang 0x00000000018bb9b2
24 clang 0x00000000018bc0b7 clang::CodeGen::CodeGenFunction::EmitCXXGlobalVarDeclInit(clang::VarDecl const&, llvm::Constant*, bool) + 437
25 clang 0x00000000018bdd1f clang::CodeGen::CodeGenFunction::GenerateCXXGlobalVarDeclInitFunc(llvm::Function*, clang::VarDecl const*, llvm::GlobalVariable*, bool) + 501
26 clang 0x00000000018bcda1 clang::CodeGen::CodeGenModule::EmitCXXGlobalVarDeclInitFunc(clang::VarDecl const*, llvm::GlobalVariable*, bool) + 615
27 clang 0x0000000001774193 clang::CodeGen::CodeGenModule::EmitGlobalVarDefinition(clang::VarDecl const*) + 2291
28 clang 0x000000000177216f clang::CodeGen::CodeGenModule::EmitGlobalDefinition(clang::GlobalDecl, llvm::GlobalValue*) + 531
29 clang 0x00000000017719ca clang::CodeGen::CodeGenModule::EmitGlobal(clang::GlobalDecl) + 796
30 clang 0x00000000017792a2 clang::CodeGen::CodeGenModule::EmitTopLevelDecl(clang::Decl*) + 438
31 clang 0x00000000016c6215
32 clang 0x00000000016aabb6
33 clang 0x00000000019ac057 clang::ParseAST(clang::Sema&, bool, bool) + 553
34 clang 0x00000000013a8fe2 clang::ASTFrontendAction::ExecuteAction() + 322
35 clang 0x00000000016ad592 clang::CodeGenAction::ExecuteAction() + 1486
36 clang 0x00000000013a8ac1 clang::FrontendAction::Execute() + 139
37 clang 0x000000000136c902 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 772
38 clang 0x000000000132dadb clang::ExecuteCompilerInvocation(clang::CompilerInstance*) + 993
39 clang 0x0000000001318d68 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) + 770
40 clang 0x0000000001327500
41 clang 0x0000000001327ae7 main + 1074
42 libc.so.6 0x00007fedcc106ec5 __libc_start_main + 245
43 clang 0x0000000001317469
Stack dump:
0. Program arguments: build/Debug+Asserts/bin/clang -cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -disable-free -main-file-name test.cpp -mrelocation-model static -mthread-model posix -mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-aliases -munwind-tables -fuse-init-array -target-cpu x86-64 -target-linker-version 2.24 -dwarf-column-info -coverage-file /tmp/- -resource-dir build/Debug+Asserts/bin/../lib/clang/3.7.0 -internal-isystem /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8 -internal-isystem /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/x86_64-linux-gnu/c++/4.8 -internal-isystem /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/x86_64-linux-gnu/c++/4.8 -internal-isystem /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/backward -internal-isystem /usr/local/include -internal-isystem build/Debug+Asserts/bin/../lib/clang/3.7.0/include -internal-externc-isystem /usr/include/x86_64-linux-gnu -internal-externc-isystem /include -internal-externc-isystem /usr/include -std=c++11 -fdeprecated-macro -fdebug-compilation-dir /tmp -ferror-limit 19 -fmessage-length 205 -mstackrealign -fobjc-runtime=gcc -fcxx-exceptions -fexceptions -fdiagnostics-show-option -o - -x c++ test.cpp

  1. <eof> parser at end of file
  2. test.cpp:4:7: LLVM IR generation of declaration 'r'
  3. test.cpp:4:7: Generating code for declaration 'r'

clang: error: unable to execute command: Aborted (core dumped)
clang: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 3.7.0 (trunk 238351)
Target: x86_64-unknown-linux-gnu
Thread model: posix
clang: note: diagnostic msg: PLEASE submit a bug report to http://llvm.org/bugs/ and include the crash backtrace, preprocessed source, and associated run script.
clang: note: diagnostic msg:


PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang: note: diagnostic msg: /tmp/test-d13456.cpp
clang: note: diagnostic msg: /tmp/test-d13456.sh
clang: note: diagnostic msg:

I think it might be possible to get rid of the NoInitExpr here, but let's not worry about that for now. Just a couple of comments and then we can go ahead with this.

lib/CodeGen/CGExprConstant.cpp
865

This case can actually happen; you can return nullptr (for now) if it does.

1122

There is no guarantee that the layout of Base matches that of the InitListExpr; you should check that here and bail out in that case.

ygao updated this revision to Diff 27247.Jun 5 2015, 5:47 PM

Thanks. I updated the patch addressing the two small comments, and also rebased to r239192.

ygao updated this revision to Diff 27248.Jun 5 2015, 5:50 PM
rsmith added a comment.Jun 5 2015, 5:56 PM

+ Bail out if the type of the ConstantStruct does not have the same
layout
+
as the type of the InitListExpr.
+ if (CGM.getTypes().ConvertType(Field->getType()) != EltInit->getType())
+ return false;

I don't think that's quite enough; even if all the types of the
ConstantStruct match the types of the fields, the fields might be at
different offsets (due to an alignment attribute). This could happen if the
original initializer initializes one member of a union and the update
expression initializes a different member. Use getStructLayout on the
DataLayout object to find the offset of the fields within the
ConstantStruct and compare them to the offset from the ASTRecordLayout
object.

ygao updated this revision to Diff 27269.Jun 6 2015, 11:04 PM

Addressing review feedback.

rsmith accepted this revision.Jun 9 2015, 1:40 PM
rsmith edited edge metadata.

Please update your llvm_unreachable("Not implemented");s to say why the code is unreachable ("should not exist in syntactic form of initializer" or similar). Other than that, let's go ahead with this change.

Thanks for your extreme patience!

This revision is now accepted and ready to land.Jun 9 2015, 1:40 PM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in rL239446.