This is an archive of the discontinued LLVM Phabricator instance.

MC/DC in LLVM Source-Based Code Coverage: clang
ClosedPublic

Authored by alanphipps on Nov 28 2022, 12:01 PM.

Details

Summary

MC/DC in LLVM Source-based Code Coverage: Review 3/3

Background
I previously upstreamed work to enable Branch Coverage (https://reviews.llvm.org/D84467), which was pushed in early 2021. MC/DC (Modified Condition/Decision Coverage) is a planned enhancement to Source-based Code Coverage. Implementation was completed in May for our downstream Arm compiler, and in general use it has not yielded any issues.

Patches have been tested on windows (stage1) and linux/mac (stage1 and stage2). Fuchsia is not yet supported.

See attached file for Background, Motivation, Design, and Implementation Concepts:

See attached PDF for Technical Talk slides from 2022 LLVM Developers' Meeting:

I am also available at Discord @alanphipps

Review
This review is for the clang front-end components.

Start with the front-end changes:

  • New Option and common parameters
    • CodeGenOptions.def
    • Options.td
    • Clang.cpp
  • Bitmap Coverage Object Allocation:
    • CodeGenPGO.h, CodeGenPGO.cpp
  • Source Region Mapping:
    • CoverageMappingGen.h, CoverageMappingGen.cpp
  • Instrumentation (during IR lowering):
    • CGExprScalar.cpp
    • CGStmt.cpp
    • CodeGenFunction.h, CodeGenFunction.cpp

Diff Detail

Event Timeline

alanphipps created this revision.Nov 28 2022, 12:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 12:01 PM
alanphipps requested review of this revision.Nov 28 2022, 12:01 PM
alanphipps edited the summary of this revision. (Show Details)Nov 28 2022, 12:15 PM

@peter.smith I know you may not feel comfortable reviewing everything or signing-off on the review, but for changes pertinent to clang, I think you made some substantive suggestions, so I am including you in the reviewer list. Thanks for your input.

alanphipps edited the summary of this revision. (Show Details)Nov 28 2022, 12:28 PM

Update diff to fix a small bug and add a test case to test intrinsic generation (based off a comment in the backend review)

I'm no expert here but since you've been waiting a while for review I'll have a go.

Since this is so testing focused it would be great to see some unit tests in addition to the integration tests you've already added.

clang/include/clang/Basic/CodeGenOptions.def
213

Looks like there's no way to set MCDCMaxConditionCount, in which case perhaps it's best to omit the option for now.

clang/include/clang/Driver/Options.td
1695

If I saw -fmcdc in a long list of command line options I'm not sure its purpose would be clear. Perhaps -fcoverage-mcdc? You could also have it imply -fcoverage-mapping.

clang/lib/CodeGen/CodeGenPGO.cpp
165

I think it would make the code more readable to put MCDC in this variable name since it's specific to MC/DC. Ditto for MaxCond.

175

Not used?

281

Should this be static_cast?
Alternatively maybe you could just dyn_cast to Expr and use that instead of isa

1186

Does this handle multiple nots? e.g. bool B = !!SomeInt.

More comments here would be appreciated to explain the intended behaviour.

I notice stripCond() does the same thing as this code so perhaps that function should be called instead of duplicating the behaviour here.

markww added a subscriber: markww.May 15 2023, 1:58 PM

Took a look and added some comments. I'm new to Clang reviews, so do let me know if I'm not following official practices for this repo.

clang/lib/CodeGen/CGExprScalar.cpp
4570

The way this code is structured is making me wonder if the if was intentionally a single-statement if or if that was an accident. Suggest adding curly braces to disambiguate or just a newline to make the grouping intent clearer.

4570

It looks like, with these push_back and pop_back MCDCLogOpStack operations, that you are essentially opening and closing contexts for tracking MC/DC concerns, and that every MC/DC push should be paired with a pop directly afterwards. Could this be made a bit easier to manage and read by having the MCDCLogOpStack instead have a MCDCOpStack method, taking as its only argument a lambda containing the code that should operate within that op context, which performs the push and pop behind the scenes? That would also hide more of the implementation details of the MCDC tracking.

So this would shift the code from looking like this:

CGF.MCDCLogOpStack.push_back(E);

/* code dealing with the current operation */

CGF.MCDCLogOpStack.pop_back(E);

To something more like this:

CGF.OpenMCDCOpContext(E, []() {
  /* code dealing with the current operation */
}

with OpenMCDCOpContext just being:

template <class ContextFn>
void OpenMCDCOpContext(const BinaryOperator *E, ContextFn fn) {
  MCDCLogOpStack.push_back(E);
  fn();
  MCDCLogOpStack.pop_back(E);
  // If the top of the logical operator nest, update the MCDC bitmap.
  if (CGF.MCDCLogOpStack.empty())
    CGF.updateMCDCTestVectorBitmap(E);
}

Alternatively, you could use an RAII object in a statement block to achieve a similar effect.

clang/lib/CodeGen/CodeGenFunction.cpp
1734

The use of this ConditionalOp parameter is very subtle and non-obvious. It may warrant surfacing some explanation of its use in this top-level method comment.

clang/lib/CodeGen/CodeGenFunction.h
1556

The naming of this function and its comment make it sound like it's definitely going to create an MCDC temporary. But, in reality, it's only going to create such a temporary if isMCDCCoverageEnabled. Perhaps this should be maybeCreateMCDCCondBitmap?

1564

It looks like this method does not modify its this object; should it formalize this with a trailing const qualifier, similar to isMCDCCoverageEnabled?

1579

Here (and in a few other methods around here) you refer to the parameter as a "Statement` and name it S, but the type is Expr. Could confusion be reduced by instead describing it as an "Expression" and naming the variable E?

clang/lib/CodeGen/CodeGenPGO.cpp
281

I'd like to +1 this. This should either use Clang's cast function instead of reinterpret_cast or both the check and the cast should be rolled into a dyn_cast like so:

if (const Expr *E = dyn_cast<Expr>(S)) {

Similar for all uses of reinterpret_cast added in this file.

291–294

Does this mean that if I tried to use this coverage functionality in a repository with split-nested conditions, the coverage tool would error out from under me? That's not a great user experience for trying to make MC/DC progress on a codebase not previously using MC/DC coverage. Perhaps split conditions can just be marked as uncovered, which would force unnesting them without stopping the tool?

Alternatively, you can see split conditions as merely passing a boolean expression to another function. That's arguably outside the scope of what MC/DC attempts to govern, and so these split conditions could be ignored and exercised only within the called function.

332

It looks like you're putting this counting of conditions inside a block that checks whether the IndexedInstrProf is greater than Version7. Is that what you want, or was this an accidental nesting? Alternatively, you could imagine writing this code like this:

bool VisitBinaryOperator(BinaryOperator *S) {
  if (S->isLogicalOp()) {
    bool lhs_is_instrumented = CodeGenFunction::isInstrumentedCondition(S->getLHS());
    bool rhs_is_instrumented = CodeGenFunction::isInstrumentedCondition(S->getRHS());
    NumCond += (lhs_is_instrumented) ? 1 : 0;
    NumCond += (rhs_is_instrumented) ? 1 : 0;
    if (ProfileVersion >= llvm::IndexedInstrProf::Version7 && rhs_is_instrumented) {
      CounterMap[S->getRHS()] = NextCounter++;    }
  }
}
1144–1148

If you just assign the iterator returned by find to a local, you can prevent looking the expression up again with operator[]. Similar for other uses of find followed by operator[] throughout.

clang/lib/CodeGen/CoverageMappingGen.cpp
113–118

These fields are named after fairly general, common concepts one might think about in source code, but are used in a fashion that is specific to MC/DC coverage. Should these be grouped in a struct layer, such as MC_DC_Metrics or something, to prevent confusion and misuse of these metrics? This may also help reduce the number of easily-confusable, primitive-typed parameters this change adds to SourceMappingRegion and similar.

182–184

MC/DC's use of "condition" and "decision" is in conflict with the general usage of "condition" in programming language discussion. For instance, I usually would say in a code review "the condition of this if statement" to mean the top-level boolean expression that MC/DC would describe as a decision. It would be useful to qualify the uses of Condition and Decision in these methods with MCDC, or make them methods of the MCDC-focused struct I recommended elsewhere.

657

Throughout the code, you track an MC/DC condition ID with the unsigned type. It might be better, to prevent accidentally confusing different pieces of data, to make an MCDC_ID class or struct, even if that just ends up containing a single unsigned field.

657–659

Unless otherwise specified, a std::stack is a std::deque under the hood ( see https://en.cppreference.com/w/cpp/container/stack ). Do you need that level of flexibility, or would a std::vector with push_back and pop_back work ok and give you better cache locality?

685–687

Rather than a C-style cast, the most unsafe of casts, you probably want Clang's cast function. Similar throughout file.

706–718

I think all of these methods can, and should, have a trailing const qualifier to make them take a const this object.

712

Why does this take a Expr * instead of a const Expr * when you're not modifying the expression?

742

This cast is completely unnecessary, as BinaryOperator has Expr as its superclass. Just remove the cast entirely, here and in similar cases throughout file.

882–895

emplace_back should only be used over push_back when absolutely necessary ( see https://abseil.io/tips/112 ). Modern C++ does a good job of eliding copies of temporaries and push_back has clearer, safer semantics than emplace_back in the face of implicit conversions. Similar throughout.

@alanphipps Haven't you forgot updating this?
Excuse me if you are working for other ongoing issues.

alanphipps marked 22 inline comments as done.Jun 15 2023, 1:11 PM
alanphipps added inline comments.
clang/include/clang/Driver/Options.td
1695

Thanks! I'm favorable toward -fcoverage-mcdc. I'm not sure I want it to imply -fcoverage-mapping because I want to reinforce that mcdc is enabled in-addition-to existing coverage, rather than instead-of, although it's debatable.

clang/lib/CodeGen/CodeGenPGO.cpp
281

Thanks -- you are both right, using dyn_cast is much better for this.

291–294

That's right -- treating it as another function would be beyond what MC/DC would track in its present function context.

I see your point, and I did previously debate the proper way to deal with the split-nest case. I opted against skipping / not covering the conditions because that information would too easily get lost in the overall coverage results; even if clang were to warn users when these cases are seen, the visualization tool has no way to express this information in the summary report in a meaningful way (though in the source view it's more clear). If we wanted to enable better support for that, it would mean marking/tracking uncovered conditions through extensions to the infrastructure, and frankly this corner case didn't seem common enough in code most pertinent to MC/DC (including embedded code coverage) to warrant that level of support, at least not yet.

I opted for the explicit error as a better user experience because to test meaningfully for MC/DC, users are going to be confronted with having to deal with these odd cases anyway -- best to make them aware of it up front.

At the end of the day, it's a corner case that (in my opinion) isn't worth adding special support for.

alanphipps added inline comments.Jun 15 2023, 1:11 PM
clang/lib/CodeGen/CGExprScalar.cpp
4570

Yes, that looks like a reasonable way to clean that up, and would certainly help this file, although it gets a bit more hairy in CodeGenFunction.cpp. I'd prefer to handle this suggestion in a subsequent patch.

clang/lib/CodeGen/CodeGenPGO.cpp
332

You're right that the instrumentation of the NextCounter for the logical operator is for >= Version7. I'll fix this to make it more clear. I did that work originally for branch coverage instrumentation, and I was thinking of MC/DC in terms of an extension to that work. But for MC/DC, this function is simply counting conditions, and so doesn't have to be guarded the same way.

clang/lib/CodeGen/CoverageMappingGen.cpp
657

A type alias may be simple enough to make the distinction.

882–895

That's a fair point.

In this case, I decided to follow the pattern for how region objects are pushed onto the RegionStack throughout the rest of the file. Without knowing why that was done (other than others just following what came before, as I did, which is probably the case), I'm reluctant to change all instances of emplace_back() to push_back() here. It might be worth a subsequent patch to clean that up.

alanphipps marked 7 inline comments as done.Jun 15 2023, 2:31 PM

@alanphipps Haven't you forgot updating this?
Excuse me if you are working for other ongoing issues.

Hi -- no I hadn't forgotten; it was just taking me a while to work through all of the comments after I updated the other reviews. I'm still working on it and will upload a new patch shortly.

I'm ok with to happy with your resolutions so far @alanphipps , but I do want to push back on that split decision error. I think that could greatly impact the usability of this tool for people who aren't starting from a clean slate.

clang/lib/CodeGen/CodeGenPGO.cpp
291–294

The big thing I'm worried about is the "incrementality" story of turning on this coverage tool if this case is treated as a capital E Error.

Imagine if I have a million lines of code that I've gotten into good shape in terms of statement coverage and I want to start measuring MC/DC coverage for this project. This code was not written with MC/DC coverage in mind, but maybe I have some automotive/aerospace/etc clients that are interested in using it and I want to give them an indication of how well it adheres to their testing standards. My first step is usually just to turn on the code to measure this new metric and see how it does.

At this point, I don't care how bad this metric is, I can fix that slowly over time. What I do care about is that I successfully get some numbers. If I get an error during this process, that's significant friction: I need to change the code to get what I want. If I get 100 errors, I probably just give up.

This is what I'm worried about with making the split-condition case a diagnostic error. From my perspective as a code owner, almost anything would be better, even marking the line as uncovered with no recourse other than to refactor, because that allows me to get numbers now and prioritize fixing it later.

I would also like to note that you're never going to get the whole class of boolean conditions that are kind of like split conditions correctly marked in MC/DC. If, for instance, this was a bitvector represented as a uint64_t containing multiple conditions that I "check" by transforming via arithmetic and bitwise expressions, that won't be handled. If this condition were wrapped in a function that happened to return the boolean expression, that also would not be handled. I think issuing a diagnostic here is a very special handling, and one that's inconsistent with the "threat model" (metaphorically) adhered to elsewhere in this PR for capturing untracked boolean expressions. The most consistent handling is either ignore this or mark it as uncovered.

alanphipps marked 3 inline comments as done.Jun 16 2023, 1:33 PM
alanphipps added inline comments.
clang/lib/CodeGen/CodeGenPGO.cpp
1186

Good catch! I modified this to ignore multiple logical-NOTs as well as parens, and I abstracted this into one function as you suggested. In fact, I'm pretty sure not handling multiple nots would've still yielded correct code, but it wouldn't have been consistent with the way conditions are handled elsewhere, and it could've been a lurking bug.

alanphipps marked 2 inline comments as done.Jun 16 2023, 1:50 PM
alanphipps added inline comments.
clang/lib/CodeGen/CodeGenPGO.cpp
291–294

This is what I'm worried about with making the split-condition case a diagnostic error. From my perspective as a code owner, almost anything would be better

OK, fair enough. While I'm not 100% aligned on this, I'm inclined to make the diagnostic a warning, which will mean that the expression will be excluded by MC/DC (i.e. ignored). It still concerns me that llvm-cov will not reflect this when reporting MC/DC coverage. llvm-cov doesn't know whether it should've been covered, so I'd like to address that in a subsequent patch -- that is, figure out a straightforward way to explicitly mark the expression (and/or its constituent conditions) as being uncovered by MC/DC so that llvm-cov can reflect that in the overall MC/DC metric.

I would also like to note that you're never going to get the whole class of boolean conditions that are kind of like split conditions correctly marked in MC/DC.

Yes, and wherever possible, I want to encourage folks to enhance these clang checks too in order to be more useful and meaningful for users attempting to use MC/DC.

alanphipps marked an inline comment as done.
alanphipps added reviewers: markww, michaelplatings.

Rebase and updated based on review comments. Thank you for all of the great suggestions!

alanphipps added inline comments.
clang/lib/CodeGen/CoverageMappingGen.cpp
113–118

What I ended up doing here was abstracting these parameters into the CounterMappingRegion class defined in CoverageMapping.h and just used that here. This reduces the coupling. The CoverageMapping updates are in the preceding review, https://reviews.llvm.org/D138847

markww accepted this revision.Jun 16 2023, 2:07 PM

This looks good to me. Thank you for changing the split condition case to a warning, I think that will be quite helpful for initial runs. It also allows people who want to scrub their codebase of split conditions to upgrade it to an error, so I think that's a pretty good user experience.

This revision is now accepted and ready to land.Jun 16 2023, 2:07 PM

@alanphipps Thanks for the update.
Could you please apply https://github.com/chapuni/llvm-project/commit/c8b8e86ed5755ff471b003e483237551b49c15ba as clang-format?

I have tried this to the llvm-project tree with the modified LLVM_BUILD_INSTRUMENTED_COVERAGE and met a few issues.

I met also a case that clang crashes with constexpr.

constexpr bool isBigEndianHost = false;

void foo() {
  if constexpr (IsBigEndianHost) {
    for (int i = 0; i < 8; i++) {}
  }
}

See, https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/SHA1.cpp#L191 and SHA256.cpp

clang/lib/CodeGen/CodeGenFunction.cpp
1259

I am afraid this would not be called in C++ ctor, dtor, etc.

clang/lib/CodeGen/CodeGenFunction.h
1572

This crashes if maybeCreateMCDCCondBitmap() was not called.

clang/lib/CodeGen/CodeGenPGO.cpp
979

I think 6 is too small for usual cases, if it would cause errors (instead of suppressions as warnings)

I applied clang-format to each of the patches at the start, but I can apply it again after the mods.

I do not agree with this. I don't think it is appropriate to track MC/DC coverage on a boolean expression with more than six conditions -- usual cases for embedded code is common 2 or 3 conditions. Eventually we could add an option to allow for more, but the function you point to has 323 conditions in one expression. On top of it being expensive to track test vectors for expressions with more conditions, it would be very difficult to generate test vectors in the first place. The intention with supporting MC/DC is not to run it on LLVM code itself, which clearly is not intended to be run on an embedded system.

  • Clang crashes if C++ ctor has conditions. I guess other kinda function-like would have same issues.

I met also a case that clang crashes with constexpr.

constexpr bool isBigEndianHost = false;

void foo() {
  if constexpr (IsBigEndianHost) {
    for (int i = 0; i < 8; i++) {}
  }
}

I will see if I can reproduce this.

The intention with supporting MC/DC is not to run it on LLVM code itself, which clearly is not intended to be run on an embedded system.

Nor, I should say, was it developed with functional safety in mind, even if it can be run on an embedded system.

I do not agree with this. I don't think it is appropriate to track MC/DC coverage on a boolean expression with more than six conditions -- usual cases for embedded code is common 2 or 3 conditions. Eventually we could add an option to allow for more, but the function you point to has 323 conditions in one expression. On top of it being expensive to track test vectors for expressions with more conditions, it would be very difficult to generate test vectors in the first place. The intention with supporting MC/DC is not to run it on LLVM code itself, which clearly is not intended to be run on an embedded system.

I can back this claim, applications undergoing MC/DC analysis would never require more than this. The fact that you found an example outside this envelope, inside the compiler, says more about LLVM itself, than this analysis. It could be interesting to see clang certified against avionics standards but I just can't see why would you even try. :D

alanphipps marked 2 inline comments as done.Jun 19 2023, 5:09 PM
alanphipps added inline comments.
clang/lib/CodeGen/CodeGenFunction.cpp
1259

Thanks for pointing this out. I enabled this and added tests. I'll upload the diff soon.

clang/lib/CodeGen/CodeGenFunction.h
1572

Thanks I will fix this.

clang/lib/CodeGen/CodeGenPGO.cpp
979

What I could do is change this to a warning as well, and eventually we let llvm-cov know that it isn't covered.

alanphipps marked 3 inline comments as done.

Updated for additional review comments (C++ constructors, destructors, 'if constexpr') and reapply clang-format.

I've found the case of assertion failure (in CoverageMapping.cpp:507, llvm-cov)

One of cases is gtest.cc:5524
It seems a Region would not be emitted if its condition consists of macro expansion (from other files).

The easiest reproduction is DemangleTests. I expect other LLVM unittests would unveil such failures.

clang/lib/CodeGen/CoverageMappingGen.cpp
447–450

Conditions aren't emitted sometimes due to this.

paquette added inline comments.
clang/lib/CodeGen/CoverageMappingGen.cpp
447–450

I think that's true in all coverage modes, not just this one. If we want to change that, I think we should change it in a separate patch since it will impact more than MC/DC.

666

Maybe use SmallVector? It's generally preferred over std::vector in the LLVM codebase.

https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h

paquette added inline comments.Jul 3 2023, 7:00 PM
clang/lib/CodeGen/CoverageMappingGen.cpp
447–450

@chapuni Could you check and see if this reproduces with non-MC/DC coverage and create a GitHub issue for it?

isZumpo added a subscriber: isZumpo.Jul 7 2023, 9:34 AM

Sorry for the delay. I am investigating RegionStack if PopRegions() would destroy branches.

clang/lib/CodeGen/CoverageMappingGen.cpp
447–450

@paquette Seems this is not the culprit. Sorry for noise.

alanphipps marked 5 inline comments as done.Jul 15 2023, 3:48 PM
alanphipps added inline comments.
clang/lib/CodeGen/CoverageMappingGen.cpp
447–450

No problem! Feel free to create an issue for it.

alanphipps marked an inline comment as done.
paquette added inline comments.Jul 17 2023, 8:52 PM
clang/lib/CodeGen/CodeGenPGO.cpp
296

Can you early-return true here? Then we can eliminate the else-after-return (https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return)

298

Somehow we should turn this into a plain if statement.

331

I think this needs clang-format.

979

So, why not have a uint max maximum here or something?

IIUC it might be expensive, but if embedded code rarely exceeds 6, then it shouldn't have a serious performance/compile-time impact, right?

Maybe it'd be good to elaborate in the comment why this is set to a small hardcoded value.

alanphipps marked 4 inline comments as done.

Update diff based clang-format and review comments...

clang/lib/CodeGen/CodeGenPGO.cpp
331

I don't see clang-format catching anything here, but I can remove the unnecessary braces.

paquette added inline comments.Aug 1 2023, 11:40 PM
compiler-rt/test/profile/ContinuousSyncMode/image-with-mcdc.c
4

Flag needs to be updated

michaelplatings resigned from this revision.Aug 2 2023, 12:05 AM
michaelplatings removed a subscriber: michaelplatings.
alanphipps updated this revision to Diff 546505.Aug 2 2023, 9:29 AM
alanphipps marked an inline comment as done.
alanphipps added a reviewer: paquette.

Updating Diff -- fix Darwin test with updated option

paquette accepted this revision.Aug 8 2023, 6:55 PM

alright this lgtm

maybe @phosek has some extra thoughts?

One thing that I haven't yet done is update clang/docs/SourceBasedCodeCoverage.rst to describe MC/DC. While I don't expect that to be difficult, if nobody objects (and unless there are additional comments), I am inclined to post a simple follow-on review for that update while I start landing the incremental functional commits here, beginning with https://reviews.llvm.org/D138846

I'm sure many of you are in the same boat with trying to manage competing project priorities, so I apologize that my updates haven't been as timely of late! But I'm very grateful for all of the assistance everyone has provided to this project.

MaskRay added inline comments.Aug 9 2023, 4:48 PM
clang/lib/CodeGen/CodeGenPGO.cpp
301

https://llvm.org/docs/CodingStandards.html#error-and-warning-messages

"start the first sentence with a lower-case letter, and finish the last sentence without a period"

311

consider std::max instead of alignTo

312

no parens here.

clang/test/CoverageMapping/mcdc-logical-stmt-ids.cpp
52

If a line mismatches, it will be difficult for a contributor that's unfamiliar with the code to fix the tests.

Consider whether some CHECKs can be changed to CHECK-NEXT: and whether some Decision,File can be changed to CHECK-LABEL:

clang/test/Profile/c-mcdc-class.cpp
8

2-space indentation is more common.

9

no spaces after ( or before )

alanphipps marked 6 inline comments as done.

Update diff based on more recent comments. Thanks! If there's nothing more, I will plan to land this in September.

Merge conflict in Options.td (D157150, D157151, D158037). Could you follow?

Would it be possible to get this landed before the Phabricator shutdown?

Would it be possible to get this landed before the Phabricator shutdown?

Yes, absolutely -- I'd like to get everything landed this month. I plan to rebase and land D138846 within the next few days to start. Thank you for your and everyone's help with this and your patience.

phosek accepted this revision.Sep 19 2023, 3:04 PM

LGTM

clang/lib/CodeGen/CodeGenPGO.cpp
1143

Nit: This name doesn't follow LLVM naming conventions.

1194

Nit: This name doesn't follow LLVM naming conventions.

This revision was landed with ongoing or failed builds.Mon, Jan 15, 1:09 PM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMon, Jan 15, 1:09 PM
Herald added subscribers: Restricted Project, cfe-commits. · View Herald Transcript