Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1309073003:
Subzero: Add a detailed design document. (Closed)
|
Created:
5 years, 3 months ago by Jim Stichnoth Modified:
5 years, 3 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionSubzero: Add a detailed design document.
This is a reStructuredText version of https://docs.google.com/a/google.com/document/d/1DmLVyZqqwWSZ0is91lipTm4xsfjInSba2KBOOxatYhg/edit?usp=sharing which for technical reasons is only visible to @google.com accounts.
Also update README.rst to be more accurate.
BUG= none
R=jfb@chromium.org, jpp@chromium.org, jvoung@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=efb89713cd549377d4746d56c8b164b50023f088
Patch Set 1 #
Total comments: 4
Patch Set 2 : Formatting, AT&T syntax, and minor changes #
Total comments: 126
Patch Set 3 : Code review changes #
Total comments: 24
Patch Set 4 : More code review changes #
Total comments: 10
Patch Set 5 : Address Jan's comments, plus one more JF comment #Messages
Total messages: 14 (2 generated)
stichnot@chromium.org changed reviewers: + dschuff@chromium.org, jfb@chromium.org, jpp@chromium.org, jvoung@chromium.org
stichnot@chromium.org changed reviewers: + kschimpf@google.com
lgtm few nits, lgmt otherwise. https://codereview.chromium.org/1309073003/diff/1/DESIGN.rst File DESIGN.rst (right): https://codereview.chromium.org/1309073003/diff/1/DESIGN.rst#newcode902 DESIGN.rst:902: mov $0x11223344, <%Reg/Mem> The example above (RMW) uses intel syntax. I'd say we should pick one style and go with it. https://codereview.chromium.org/1309073003/diff/1/DESIGN.rst#newcode1170 DESIGN.rst:1170: ARM32 work is in progress and is mostly complete. It currently lacks the As Jan pointed out there's substantial amount of work for ARM32. I would just say "... in progress." For ARM64 I don't think we will ever have a sandbox model.
https://codereview.chromium.org/1309073003/diff/1/DESIGN.rst File DESIGN.rst (right): https://codereview.chromium.org/1309073003/diff/1/DESIGN.rst#newcode902 DESIGN.rst:902: mov $0x11223344, <%Reg/Mem> On 2015/08/31 15:02:18, John wrote: > The example above (RMW) uses intel syntax. I'd say we should pick one style and > go with it. Switched everything to AT&T syntax. https://codereview.chromium.org/1309073003/diff/1/DESIGN.rst#newcode1170 DESIGN.rst:1170: ARM32 work is in progress and is mostly complete. It currently lacks the On 2015/08/31 15:02:18, John wrote: > As Jan pointed out there's substantial amount of work for ARM32. I would just > say "... in progress." For ARM64 I don't think we will ever have a sandbox > model. Done.
https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst File DESIGN.rst (right): https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode9 DESIGN.rst:9: developer uses the PNaCl toolchain to compile their application to "web program developer" sounds weird. Just "developer" is cleaner. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode10 DESIGN.rst:10: architecture-neutral PNaCl bitcode (a pexe file), using as much Code-quote ``.pexe``, here and elsewhere. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode13 DESIGN.rst:13: the pexe file to sandboxed native code. The translator uses Link "sandboxed" to: https://developer.chrome.com/native-client/reference/sandbox_internals/index https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode18 DESIGN.rst:18: future page loads. However, first-time user experience can be hampered by long s/can be/is/ https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode26 DESIGN.rst:26: e.g. ARM chromebooks, are likely to have fewer cores to parallelize across. Also, RAM is limited. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode34 DESIGN.rst:34: downloading large assets. Arrange the web page to distract the user with cat [citation needed] Authoritative source: https://www.youtube.com/results?search_sort=video_view_count&search_query=cut... https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode46 DESIGN.rst:46: arbitrarily say 1 MB/sec. We'll pick an ARM A15 platform as the example of a s/an/the/ s/platform/CPU/ https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode47 DESIGN.rst:47: slow machine. We observe a 3x single-thread performance difference between A15 Unicode has a most wonderful multiplication sign for your consideration: × https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode48 DESIGN.rst:48: and a z620 workstation, and aggressively assume a pexe file could be compressed s/z620/high-end x86 Xeon E5-2690/ https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode49 DESIGN.rst:49: to 50% on the web server before network transport, so we set the translation "using gzip transport compression" https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode53 DESIGN.rst:53: 1/10 the target rate. The ``-O2`` mode takes 3x as long as the ``-O0`` mode. ⅒ https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode63 DESIGN.rst:63: average, LLVM ``-O2`` performs twice as well as LLVM ``-O0``. You should re-emphasize that target-independent optimizations are assumed to already have been performed. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode68 DESIGN.rst:68: The current LLVM-based translator binary (pnacl-llc) is about 10MB in size. We Code-quotes ``pnacl-llc`` https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode75 DESIGN.rst:75: features are compiled out. This section is a goal... but it doesn't really say *why* this is a useful goal. Being distributed to over a billion users would be a good reason to save 9MB :-) https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode83 DESIGN.rst:83: can lead to the translator process holding hundreds of MB of memory by the end. This doesn't really say *why*. You should probably note that subzero runs in its own process, so it's not about sharing virtual memory with other Chrome processes, but rather about pilfering physical memory. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode89 DESIGN.rst:89: test for leaks.) O(largest-function-size) instead of O(total-function-count * average-function-size). https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode127 DESIGN.rst:127: Intermediate representation - ICE I'd expect to read "SSA" pretty early in the detailed compiler design. I see CFG, but not SSA! Also, mention that code generation is done method-at-a-time. It would also be nice to have a quick sequence diagram of the compiler's phases here. I also don't see a mention of what you do with globals. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode133 DESIGN.rst:133: A function is represented by the ``Ice::Cfg`` class (CFG = control flow graph). "A function's Control Flow Graph (CFG) is represented by the ``Ice::Cfg`` class." https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode181 DESIGN.rst:181: - Alloca: allocate data on the stack These are more restricted than LLVM's though? This applies to other instructions too, would it be worth to explain differences somewhere? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode235 DESIGN.rst:235: constant of a particular type), and symbol constants (essentially addresses of SIMD constants? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode286 DESIGN.rst:286: - Parse the next function from the pexe file and construct a Cfg consisting of Capitalize CFG since it's an acronym, not code. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode313 DESIGN.rst:313: Debuggability of generated code was never considered in the design, but so far, "never" seems pretty strong! Especially considering this document is DESIGN.rst, so it does seem to consider debuggability, and choose to defer it for now ;-) https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode319 DESIGN.rst:319: nor does it coalesce stack slots. This should discuss what's needed for debuggability (emitting DWARF debug info), and what PNaCl currently does for comparison. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode333 DESIGN.rst:333: Pexe parsing It seems more intuitive to have this earlier, since parsing happens first. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode352 DESIGN.rst:352: instructions, in a largely context-free setting. That is, each high-level a.k.a. "splat" or "template" JIT. It would be good to quote other systems which do this, e.g. QEMU or Valgrind to show that this isn't just a random thing Subzero does. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode448 DESIGN.rst:448: - Register allocation (infinite-weight and pre-colored only) There's (part of) the pipeline that I wish I'd seen early on! Could you make this a pipeline (with a fork for O2/Om1) and put it up top? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode587 DESIGN.rst:587: this second-chance mechanism is disabled. Could you mention greedy RA, and why you may want to try it out? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode746 DESIGN.rst:746: Sandboxing You should explain why sandboxing is needed (NaCl) and how Subzero is great because it means we can experiments with restrictions on NaCl, use features that aren't on every CPU, and even try out sandboxes other than NaCl's. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode771 DESIGN.rst:771: Subzero's integrated assembler is derived from Dart's assembler code. There is Link? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode794 DESIGN.rst:794: Where possible, we allocate from a CfgLocalAllocator which derives from LLVM's Code-quote ``CfgLocalAllocator`` and others. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode797 DESIGN.rst:797: and the Cfg is deleted, the entire arena memory is reclaimed at once. Why does Subzero do this? I know why, but the document should tell me! https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode801 DESIGN.rst:801: all instructions from the CfgLoclaAllocator, and we make sure each instruction Typo on CfgLoclaAllocator. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode802 DESIGN.rst:802: subclass is basically POD with a trivial destructor. This way, when the Cfg is Link that explains POD. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode806 DESIGN.rst:806: There are some situations where passes need to use some STL container class. Link that explains STL. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode810 DESIGN.rst:810: Multithreaded translation This section should mention what the scaling w.r.t. N is. "Close to linear"? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode832 DESIGN.rst:832: Note that ``-threads=N`` actually creates N+2 threads. For the special case of I'd rephrase to "There are therefore N+2 threads spawned when taking into account parse and assembly threads." https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode843 DESIGN.rst:843: Security This section doesn't really clarify that all these mitigations are on top of the NaCl sandbox as well as Chrome's OS-level sandbox. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode843 DESIGN.rst:843: Security This section should list other things that Subzero can do to improve security. It can tighten the NaCl sandbox, e.g. x86-64 base address hiding. Making code X only. Instruction selection diversification. Use dfsan in production! (loop in pcc) ...? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode853 DESIGN.rst:853: Chrome sandbox. I'd hope it's more secure! You should mention that LLVM wasn't written with security as a goal, whereas Subzero is. Much depth, such defense, wow. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode855 DESIGN.rst:855: Code diversification This section should explain that code is diversified once per translation, and that translations are cached. This means that the same user gets a single binary (unless the cache is evicted), but an attacker can't repeatedly attempt an exploit and crash because Chrome throttles crashy process creation. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode855 DESIGN.rst:855: Code diversification Explain that code diversification can be done in a repeatable manner, and document what the approach is to get this determinism. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode858 DESIGN.rst:858: Return-oriented programming (ROP) is a now-common technique for starting with Link to papers about ROP. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode862 DESIGN.rst:862: return. If the attacker can manage to overwrite parts of the stack, he can Ungendered: s/he/they/ https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode874 DESIGN.rst:874: access to them. There's a distinction to be made between securing the user from malicious developers, versus securing developer's code to prevent them from exhibiting behavior the developer didn't intend. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode880 DESIGN.rst:880: than others. Link to Matasano JIT hardening paper. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode891 DESIGN.rst:891: discussion of each technique, its effectiveness, and its cost. What's the total reduction in gadget persistence when all 7 are combined? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode893 DESIGN.rst:893: Constant blinding What's the payoff of this in reducing gadget persistence? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode917 DESIGN.rst:917: validator bug were discovered). Clarify that immediates aren't super useful in NaCl because of bundling. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode919 DESIGN.rst:919: Constant pooling What's the payoff of this in reducing gadget persistence? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode961 DESIGN.rst:961: persistence to less than 5%. To less that 5% on its own? Or combined? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode996 DESIGN.rst:996: Nop insertion is very effective in reducing ROP gadget persistence, at the same How effective? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode999 DESIGN.rst:999: block randomization instead. Don't they combine, though? Or do they interfere / not complement each other? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1033 DESIGN.rst:1033: Fuzzing This is under code diversification, but isn't a diversification. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1036 DESIGN.rst:1036: We have started fuzzing the pexe files input to Subzero. Most of the problems Using afl-fuzz, libFuzzer, and custom tooling. Links? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1040 DESIGN.rst:1040: dig into this area. This section should explain what's the point of fuzzing Subzero. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1047 DESIGN.rst:1047: manifest specifies the ``O0`` optimization level. Link to NaCl's documentation about manifest files. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1103 DESIGN.rst:1103: For basic testing, Subzero uses LLVM's lit framework for running tests. We have Link to lit docs. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1156 DESIGN.rst:1156: We regularly run Subzero with asan and tsan. So far, multithreading has been "regularly" why not bots? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1156 DESIGN.rst:1156: We regularly run Subzero with asan and tsan. So far, multithreading has been Why not ubsan? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1242 DESIGN.rst:1242: size of the translation queue to N entries - if it is "full" when the parser Em dash. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1254 DESIGN.rst:1254: throttle the translation queue to prevent too many in-flight large functions. It may be useful to get a memory pressure signal from Chrome. https://codereview.chromium.org/1309073003/diff/20001/README.rst File README.rst (right): https://codereview.chromium.org/1309073003/diff/20001/README.rst#newcode163 README.rst:163: native mode only, for the x32 flavor due to PNaCl bitcode restrictions. ARM and "in native mode only" but sandboxing is in progress? "for the x32 flavor due to PNaCl bitcode restrictions" that's not very clear.
https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst File DESIGN.rst (right): https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode9 DESIGN.rst:9: developer uses the PNaCl toolchain to compile their application to On 2015/08/31 21:08:01, JF wrote: > "web program developer" sounds weird. Just "developer" is cleaner. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode10 DESIGN.rst:10: architecture-neutral PNaCl bitcode (a pexe file), using as much On 2015/08/31 21:08:01, JF wrote: > Code-quote ``.pexe``, here and elsewhere. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode13 DESIGN.rst:13: the pexe file to sandboxed native code. The translator uses On 2015/08/31 21:08:00, JF wrote: > Link "sandboxed" to: > https://developer.chrome.com/native-client/reference/sandbox_internals/index Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode18 DESIGN.rst:18: future page loads. However, first-time user experience can be hampered by long On 2015/08/31 21:08:01, JF wrote: > s/can be/is/ Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode26 DESIGN.rst:26: e.g. ARM chromebooks, are likely to have fewer cores to parallelize across. On 2015/08/31 21:08:02, JF wrote: > Also, RAM is limited. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode34 DESIGN.rst:34: downloading large assets. Arrange the web page to distract the user with cat On 2015/08/31 21:08:01, JF wrote: > [citation needed] > > Authoritative source: > https://www.youtube.com/results?search_sort=video_view_count&search_query=cut... Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode46 DESIGN.rst:46: arbitrarily say 1 MB/sec. We'll pick an ARM A15 platform as the example of a On 2015/08/31 21:07:59, JF wrote: > s/an/the/ > s/platform/CPU/ Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode47 DESIGN.rst:47: slow machine. We observe a 3x single-thread performance difference between A15 On 2015/08/31 21:08:01, JF wrote: > Unicode has a most wonderful multiplication sign for your consideration: × Done. (here and below) https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode48 DESIGN.rst:48: and a z620 workstation, and aggressively assume a pexe file could be compressed On 2015/08/31 21:08:01, JF wrote: > s/z620/high-end x86 Xeon E5-2690/ Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode49 DESIGN.rst:49: to 50% on the web server before network transport, so we set the translation On 2015/08/31 21:07:59, JF wrote: > "using gzip transport compression" Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode53 DESIGN.rst:53: 1/10 the target rate. The ``-O2`` mode takes 3x as long as the ``-O0`` mode. On 2015/08/31 21:08:01, JF wrote: > ⅒ Done. I must work in a "pile of poo" reference somewhere. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode63 DESIGN.rst:63: average, LLVM ``-O2`` performs twice as well as LLVM ``-O0``. On 2015/08/31 21:08:02, JF wrote: > You should re-emphasize that target-independent optimizations are assumed to > already have been performed. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode68 DESIGN.rst:68: The current LLVM-based translator binary (pnacl-llc) is about 10MB in size. We On 2015/08/31 21:08:00, JF wrote: > Code-quotes ``pnacl-llc`` Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode75 DESIGN.rst:75: features are compiled out. On 2015/08/31 21:07:59, JF wrote: > This section is a goal... but it doesn't really say *why* this is a useful goal. > Being distributed to over a billion users would be a good reason to save 9MB :-) Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode83 DESIGN.rst:83: can lead to the translator process holding hundreds of MB of memory by the end. On 2015/08/31 21:07:59, JF wrote: > This doesn't really say *why*. > > You should probably note that subzero runs in its own process, so it's not about > sharing virtual memory with other Chrome processes, but rather about pilfering > physical memory. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode89 DESIGN.rst:89: test for leaks.) On 2015/08/31 21:07:59, JF wrote: > O(largest-function-size) instead of O(total-function-count * > average-function-size). Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode127 DESIGN.rst:127: Intermediate representation - ICE On 2015/08/31 21:07:59, JF wrote: > I'd expect to read "SSA" pretty early in the detailed compiler design. I see > CFG, but not SSA! > > Also, mention that code generation is done method-at-a-time. > > It would also be nice to have a quick sequence diagram of the compiler's phases > here. > > I also don't see a mention of what you do with globals. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode133 DESIGN.rst:133: A function is represented by the ``Ice::Cfg`` class (CFG = control flow graph). On 2015/08/31 21:08:01, JF wrote: > "A function's Control Flow Graph (CFG) is represented by the ``Ice::Cfg`` > class." Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode181 DESIGN.rst:181: - Alloca: allocate data on the stack On 2015/08/31 21:08:00, JF wrote: > These are more restricted than LLVM's though? This applies to other instructions > too, would it be worth to explain differences somewhere? Done, by pointing to the PNaCl ABI manual for the differences. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode235 DESIGN.rst:235: constant of a particular type), and symbol constants (essentially addresses of On 2015/08/31 21:08:00, JF wrote: > SIMD constants? Clarified that integer/FP constants are scalar, and Undef can include vector. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode286 DESIGN.rst:286: - Parse the next function from the pexe file and construct a Cfg consisting of On 2015/08/31 21:07:59, JF wrote: > Capitalize CFG since it's an acronym, not code. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode313 DESIGN.rst:313: Debuggability of generated code was never considered in the design, but so far, On 2015/08/31 21:08:00, JF wrote: > "never" seems pretty strong! Especially considering this document is DESIGN.rst, > so it does seem to consider debuggability, and choose to defer it for now ;-) Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode319 DESIGN.rst:319: nor does it coalesce stack slots. On 2015/08/31 21:08:01, JF wrote: > This should discuss what's needed for debuggability (emitting DWARF debug info), > and what PNaCl currently does for comparison. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode333 DESIGN.rst:333: Pexe parsing On 2015/08/31 21:07:59, JF wrote: > It seems more intuitive to have this earlier, since parsing happens first. Good point, I swapped it with the previous sub*section. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode352 DESIGN.rst:352: instructions, in a largely context-free setting. That is, each high-level On 2015/08/31 21:08:00, JF wrote: > a.k.a. "splat" or "template" JIT. It would be good to quote other systems which > do this, e.g. QEMU or Valgrind to show that this isn't just a random thing > Subzero does. "template" is already mentioned plenty in my text, and "splat" doesn't seem to be very common usage. I added an unsubstantiated claim that other JITs/interpreters use this template approach, plus the discussion that we defer register allocation and stack slot assignment and address mode selection until after global analysis. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode448 DESIGN.rst:448: - Register allocation (infinite-weight and pre-colored only) On 2015/08/31 21:08:02, JF wrote: > There's (part of) the pipeline that I wish I'd seen early on! > > Could you make this a pipeline (with a fork for O2/Om1) and put it up top? Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode587 DESIGN.rst:587: this second-chance mechanism is disabled. On 2015/08/31 21:08:00, JF wrote: > Could you mention greedy RA, and why you may want to try it out? Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode746 DESIGN.rst:746: Sandboxing On 2015/08/31 21:07:59, JF wrote: > You should explain why sandboxing is needed (NaCl) and how Subzero is great > because it means we can experiments with restrictions on NaCl, use features that > aren't on every CPU, and even try out sandboxes other than NaCl's. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode771 DESIGN.rst:771: Subzero's integrated assembler is derived from Dart's assembler code. There is On 2015/08/31 21:08:01, JF wrote: > Link? Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode794 DESIGN.rst:794: Where possible, we allocate from a CfgLocalAllocator which derives from LLVM's On 2015/08/31 21:08:00, JF wrote: > Code-quote ``CfgLocalAllocator`` and others. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode797 DESIGN.rst:797: and the Cfg is deleted, the entire arena memory is reclaimed at once. On 2015/08/31 21:08:00, JF wrote: > Why does Subzero do this? I know why, but the document should tell me! Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode801 DESIGN.rst:801: all instructions from the CfgLoclaAllocator, and we make sure each instruction On 2015/08/31 21:08:01, JF wrote: > Typo on CfgLoclaAllocator. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode802 DESIGN.rst:802: subclass is basically POD with a trivial destructor. This way, when the Cfg is On 2015/08/31 21:08:00, JF wrote: > Link that explains POD. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode806 DESIGN.rst:806: There are some situations where passes need to use some STL container class. On 2015/08/31 21:07:59, JF wrote: > Link that explains STL. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode810 DESIGN.rst:810: Multithreaded translation On 2015/08/31 21:08:00, JF wrote: > This section should mention what the scaling w.r.t. N is. "Close to linear"? Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode832 DESIGN.rst:832: Note that ``-threads=N`` actually creates N+2 threads. For the special case of On 2015/08/31 21:08:01, JF wrote: > I'd rephrase to "There are therefore N+2 threads spawned when taking into > account parse and assembly threads." Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode843 DESIGN.rst:843: Security On 2015/08/31 21:07:59, JF wrote: > This section doesn't really clarify that all these mitigations are on top of the > NaCl sandbox as well as Chrome's OS-level sandbox. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode843 DESIGN.rst:843: Security On 2015/08/31 21:08:02, JF wrote: > This section should list other things that Subzero can do to improve security. > > It can tighten the NaCl sandbox, e.g. x86-64 base address hiding. > > Making code X only. > > Instruction selection diversification. > > Use dfsan in production! (loop in pcc) > > ...? Done. (future work section below) https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode853 DESIGN.rst:853: Chrome sandbox. On 2015/08/31 21:08:02, JF wrote: > I'd hope it's more secure! You should mention that LLVM wasn't written with > security as a goal, whereas Subzero is. Much depth, such defense, wow. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode855 DESIGN.rst:855: Code diversification On 2015/08/31 21:08:01, JF wrote: > Explain that code diversification can be done in a repeatable manner, and > document what the approach is to get this determinism. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode855 DESIGN.rst:855: Code diversification On 2015/08/31 21:08:00, JF wrote: > This section should explain that code is diversified once per translation, and > that translations are cached. This means that the same user gets a single binary > (unless the cache is evicted), but an attacker can't repeatedly attempt an > exploit and crash because Chrome throttles crashy process creation. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode858 DESIGN.rst:858: Return-oriented programming (ROP) is a now-common technique for starting with On 2015/08/31 21:08:02, JF wrote: > Link to papers about ROP. I don't know of canonical overview papers, so how about a Wikipedia link in the meantime? https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode862 DESIGN.rst:862: return. If the attacker can manage to overwrite parts of the stack, he can On 2015/08/31 21:08:01, JF wrote: > Ungendered: s/he/they/ Mission accomplished, without resorting to agrammatical "they". https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode874 DESIGN.rst:874: access to them. On 2015/08/31 21:08:00, JF wrote: > There's a distinction to be made between securing the user from malicious > developers, versus securing developer's code to prevent them from exhibiting > behavior the developer didn't intend. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode880 DESIGN.rst:880: than others. On 2015/08/31 21:08:00, JF wrote: > Link to Matasano JIT hardening paper. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode891 DESIGN.rst:891: discussion of each technique, its effectiveness, and its cost. On 2015/08/31 21:08:00, JF wrote: > What's the total reduction in gadget persistence when all 7 are combined? Unknown at this point (and clarified that it's unknown at this point). https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode893 DESIGN.rst:893: Constant blinding On 2015/08/31 21:08:01, JF wrote: > What's the payoff of this in reducing gadget persistence? See below - "It does little to reduce gadget persistence..." under the assumption that the validator prevents intra-instruction branches. And BTW, it also has little/no effect on spec2k if you *do* allow for non-bundle-aligned ROP gadgets, simply because spec2k happens to lack such special attacker-controlled constants. In this document, I'd prefer not to dive into a full treatment of this arms race. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode917 DESIGN.rst:917: validator bug were discovered). On 2015/08/31 21:08:00, JF wrote: > Clarify that immediates aren't super useful in NaCl because of bundling. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode919 DESIGN.rst:919: Constant pooling On 2015/08/31 21:08:00, JF wrote: > What's the payoff of this in reducing gadget persistence? Same comment as for constant blinding. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode961 DESIGN.rst:961: persistence to less than 5%. On 2015/08/31 21:08:01, JF wrote: > To less that 5% on its own? Or combined? On its own, as clarified in the section intro. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode996 DESIGN.rst:996: Nop insertion is very effective in reducing ROP gadget persistence, at the same On 2015/08/31 21:08:00, JF wrote: > How effective? "at the same level as basic block randomization" :) Pasted in the 5% figure from above. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode999 DESIGN.rst:999: block randomization instead. On 2015/08/31 21:08:00, JF wrote: > Don't they combine, though? Or do they interfere / not complement each other? Clarified as unknown at this point. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1033 DESIGN.rst:1033: Fuzzing On 2015/08/31 21:08:01, JF wrote: > This is under code diversification, but isn't a diversification. Actually it's under Security - using "^^^^" instead of "~~~~". https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1036 DESIGN.rst:1036: We have started fuzzing the pexe files input to Subzero. Most of the problems On 2015/08/31 21:08:00, JF wrote: > Using afl-fuzz, libFuzzer, and custom tooling. > > Links? Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1040 DESIGN.rst:1040: dig into this area. On 2015/08/31 21:08:01, JF wrote: > This section should explain what's the point of fuzzing Subzero. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1047 DESIGN.rst:1047: manifest specifies the ``O0`` optimization level. On 2015/08/31 21:08:00, JF wrote: > Link to NaCl's documentation about manifest files. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1103 DESIGN.rst:1103: For basic testing, Subzero uses LLVM's lit framework for running tests. We have On 2015/08/31 21:08:01, JF wrote: > Link to lit docs. Done. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1156 DESIGN.rst:1156: We regularly run Subzero with asan and tsan. So far, multithreading has been On 2015/08/31 21:08:01, JF wrote: > "regularly" why not bots? That would be my failing. I reworded the "regularly run" language. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1156 DESIGN.rst:1156: We regularly run Subzero with asan and tsan. So far, multithreading has been On 2015/08/31 21:08:01, JF wrote: > Why not ubsan? Good point. I stumbled around with msan for a bit until I concluded that all the libraries would need to be rebuilt as well, but I didn't try ubsan yet. I can update the document when ubsan support is in place (and any bugs fixed). https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1242 DESIGN.rst:1242: size of the translation queue to N entries - if it is "full" when the parser On 2015/08/31 21:07:59, JF wrote: > Em dash. Made it consistent with the rest of the doc, but I'm not sure if that's the best way overall. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode1254 DESIGN.rst:1254: throttle the translation queue to prevent too many in-flight large functions. On 2015/08/31 21:08:01, JF wrote: > It may be useful to get a memory pressure signal from Chrome. Done. https://codereview.chromium.org/1309073003/diff/20001/README.rst File README.rst (right): https://codereview.chromium.org/1309073003/diff/20001/README.rst#newcode163 README.rst:163: native mode only, for the x32 flavor due to PNaCl bitcode restrictions. ARM and On 2015/08/31 21:08:02, JF wrote: > "in native mode only" but sandboxing is in progress? > > "for the x32 flavor due to PNaCl bitcode restrictions" that's not very clear. Done.
https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst File DESIGN.rst (right): https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode34 DESIGN.rst:34: downloading large assets. Arrange the web page to distract the user with cat On 2015/09/02 23:35:03, stichnot wrote: > On 2015/08/31 21:08:01, JF wrote: > > [citation needed] > > > > Authoritative source: > > > https://www.youtube.com/results?search_sort=video_view_count&search_query=cut... > > Done. Most wonderful choice. https://codereview.chromium.org/1309073003/diff/20001/DESIGN.rst#newcode333 DESIGN.rst:333: Pexe parsing On 2015/09/02 23:35:05, stichnot wrote: > On 2015/08/31 21:07:59, JF wrote: > > It seems more intuitive to have this earlier, since parsing happens first. > > Good point, I swapped it with the previous sub*section. * = zero? As in, subzerosection! https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst File DESIGN.rst (right): https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode32 DESIGN.rst:32: This doesn't help much when translation speed is 10x slower than download 10× https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode48 DESIGN.rst:48: selection of fast optimization passes. It two optimization recipes: full "It two optimization recipes" It no Englishy. https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode95 DESIGN.rst:95: workstation. on the Xeon workstation https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode122 DESIGN.rst:122: might be distributed to a billion Chrome users. Thus we target a 10× reduction Not just might! It *is* distributed to over a billion users! https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode140 DESIGN.rst:140: systems. "bloat, and in the extreme case out-of-memory tab killing" https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode309 DESIGN.rst:309: no reason to represent vector-type constants internally.. A variable represents Too many dots!! https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode840 DESIGN.rst:840: provide safety when running untrusted code in a browser or other environment. Drop "help". https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode841 DESIGN.rst:841: Subzero implements Native Client's sandboxing to enable Subzero-translated Re-link to https://developer.chrome.com/native-client/reference/sandbox_internals/index https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode914 DESIGN.rst:914: ``CfgLocalAllocator`` as the container allocator if this is needed. This STL mention reminds me: you don't explain how subzero uses LLVM! It's built as a separate tool, which only uses the stand-alone LLVM utility library ADT. I don't think this is the right place for it, but it should be somewhere in the document. https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode1211 DESIGN.rst:1211: after code diversification has been performed. Though this is still susceptible to "blind ROP" (link), it would substantially improve security. https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode1215 DESIGN.rst:1215: for code randomization. Mention using dataflow sanitizer as another improvement.
https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst File DESIGN.rst (right): https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode32 DESIGN.rst:32: This doesn't help much when translation speed is 10x slower than download On 2015/09/03 00:35:21, JF wrote: > 10× Done. https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode48 DESIGN.rst:48: selection of fast optimization passes. It two optimization recipes: full On 2015/09/03 00:35:21, JF wrote: > "It two optimization recipes" > It no Englishy. Doed. https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode95 DESIGN.rst:95: workstation. On 2015/09/03 00:35:21, JF wrote: > on the Xeon workstation Done. https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode122 DESIGN.rst:122: might be distributed to a billion Chrome users. Thus we target a 10× reduction On 2015/09/03 00:35:21, JF wrote: > Not just might! It *is* distributed to over a billion users! Done. https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode140 DESIGN.rst:140: systems. On 2015/09/03 00:35:21, JF wrote: > "bloat, and in the extreme case out-of-memory tab killing" Done. https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode309 DESIGN.rst:309: no reason to represent vector-type constants internally.. A variable represents On 2015/09/03 00:35:21, JF wrote: > Too many dots!! Done... https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode840 DESIGN.rst:840: provide safety when running untrusted code in a browser or other environment. On 2015/09/03 00:35:21, JF wrote: > Drop "help". Done. https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode841 DESIGN.rst:841: Subzero implements Native Client's sandboxing to enable Subzero-translated On 2015/09/03 00:35:21, JF wrote: > Re-link to > https://developer.chrome.com/native-client/reference/sandbox_internals/index Done. https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode914 DESIGN.rst:914: ``CfgLocalAllocator`` as the container allocator if this is needed. On 2015/09/03 00:35:21, JF wrote: > This STL mention reminds me: you don't explain how subzero uses LLVM! It's built > as a separate tool, which only uses the stand-alone LLVM utility library ADT. > > I don't think this is the right place for it, but it should be somewhere in the > document. Good point, and I agree there's not an obvious place to mention it. I mentioned the ADT and Support stuff as part of the translator size discussion, and a mention of including the IR in a non-MINIMAL build. https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode1211 DESIGN.rst:1211: after code diversification has been performed. On 2015/09/03 00:35:21, JF wrote: > Though this is still susceptible to "blind ROP" (link), it would substantially > improve security. Cool. https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode1215 DESIGN.rst:1215: for code randomization. On 2015/09/03 00:35:21, JF wrote: > Mention using dataflow sanitizer as another improvement. Before doing that, what are your thoughts on how it could be used? What aspect of Subzero or its generated code would be getting the taint analysis?
https://codereview.chromium.org/1309073003/diff/60001/DESIGN.rst File DESIGN.rst (right): https://codereview.chromium.org/1309073003/diff/60001/DESIGN.rst#newcode418 DESIGN.rst:418: would need to not strip these symbols to make it most useful. If you take the pexe with symbols and emit dwarf, it seems that by "high-level debuggability", you don't mean "source-level" (or, that you would consider the pexe as the "source" and not the c/c++ code). To debug at the c/c++ level, you would need something like the llvm.dbg intrinsics and metadata that we strip out too to actually have line numbers in stack traces, track inlining, track source variables (which have undergone some alpha-renaming and ssa conversion, etc.). https://codereview.chromium.org/1309073003/diff/60001/DESIGN.rst#newcode442 DESIGN.rst:442: be optimized templates, e.g. to take advantage of operator commutativity. This related to optimized templates: things like bool folding? https://codereview.chromium.org/1309073003/diff/60001/DESIGN.rst#newcode505 DESIGN.rst:505: Loop nest analysis too, but I guess that's still in progress. https://codereview.chromium.org/1309073003/diff/60001/DESIGN.rst#newcode881 DESIGN.rst:881: always use long offsets.) The assembler emits into a staging buffer. Once Technically, some known-near forward branches injected by lowering will use short offsets. It's the forward branches come from the original CFG that harder to predict and "always use long offsets". https://codereview.chromium.org/1309073003/diff/60001/DESIGN.rst#newcode1347 DESIGN.rst:1347: support. This is done using something as simple as ``make ASAN=1 TSAN=1``. So I don't they can analyze both simultaneously =) "clang: error: invalid argument '-fsanitize=address' not allowed with '-fsanitize=thread'"
A last comment, and then much lgtm (assuming jvoung is also happy). I quite like this design doc :-) https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst File DESIGN.rst (right): https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode1215 DESIGN.rst:1215: for code randomization. On 2015/09/03 06:47:03, stichnot wrote: > On 2015/09/03 00:35:21, JF wrote: > > Mention using dataflow sanitizer as another improvement. > > Before doing that, what are your thoughts on how it could be used? What aspect > of Subzero or its generated code would be getting the taint analysis? Oops, I meant this: http://clang.llvm.org/docs/ControlFlowIntegrity.html I'm confusing the names of two of Peter's projects, though I now realize that their names are actually quite descriptive.
https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst File DESIGN.rst (right): https://codereview.chromium.org/1309073003/diff/40001/DESIGN.rst#newcode1215 DESIGN.rst:1215: for code randomization. On 2015/09/03 16:18:50, JF wrote: > On 2015/09/03 06:47:03, stichnot wrote: > > On 2015/09/03 00:35:21, JF wrote: > > > Mention using dataflow sanitizer as another improvement. > > > > Before doing that, what are your thoughts on how it could be used? What > aspect > > of Subzero or its generated code would be getting the taint analysis? > > Oops, I meant this: > http://clang.llvm.org/docs/ControlFlowIntegrity.html > > I'm confusing the names of two of Peter's projects, though I now realize that > their names are actually quite descriptive. Ah, much better. Added essentially a TODO for that. https://codereview.chromium.org/1309073003/diff/60001/DESIGN.rst File DESIGN.rst (right): https://codereview.chromium.org/1309073003/diff/60001/DESIGN.rst#newcode418 DESIGN.rst:418: would need to not strip these symbols to make it most useful. On 2015/09/03 14:40:25, jvoung wrote: > If you take the pexe with symbols and emit dwarf, it seems that by "high-level > debuggability", you don't mean "source-level" (or, that you would consider the > pexe as the "source" and not the c/c++ code). > > To debug at the c/c++ level, you would need something like the llvm.dbg > intrinsics and metadata that we strip out too to actually have line numbers in > stack traces, track inlining, track source variables (which have undergone some > alpha-renaming and ssa conversion, etc.). Oh, excellent point. I added that to the discussion. https://codereview.chromium.org/1309073003/diff/60001/DESIGN.rst#newcode442 DESIGN.rst:442: be optimized templates, e.g. to take advantage of operator commutativity. This On 2015/09/03 14:40:25, jvoung wrote: > related to optimized templates: things like bool folding? Bool folding is one place, but I added a simpler example instead -- x=x+1 being potentially simpler than x=1+x (ignoring the non-SSA aspect). https://codereview.chromium.org/1309073003/diff/60001/DESIGN.rst#newcode505 DESIGN.rst:505: On 2015/09/03 14:40:25, jvoung wrote: > Loop nest analysis too, but I guess that's still in progress. Done. https://codereview.chromium.org/1309073003/diff/60001/DESIGN.rst#newcode881 DESIGN.rst:881: always use long offsets.) The assembler emits into a staging buffer. Once On 2015/09/03 14:40:25, jvoung wrote: > Technically, some known-near forward branches injected by lowering will use > short offsets. It's the forward branches come from the original CFG that harder > to predict and "always use long offsets". Done. https://codereview.chromium.org/1309073003/diff/60001/DESIGN.rst#newcode1347 DESIGN.rst:1347: support. This is done using something as simple as ``make ASAN=1 TSAN=1``. So On 2015/09/03 14:40:25, jvoung wrote: > I don't they can analyze both simultaneously =) > > "clang: error: invalid argument '-fsanitize=address' not allowed with > '-fsanitize=thread'" Oops! Split this into two examples.
LGTM too
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as efb89713cd549377d4746d56c8b164b50023f088 (presubmit successful). |
