[PATCH] D54604: Automatic variable initialization

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

I've addressed @pcc's comments. I'll now update the patch to mandate a `-Xclang` option for zero-init (otherwise it'll warn and pattern-init instead), and remove documentation for zero-init. I'm also thinking of changing float init to negative NaN with infinite scream payload.



================
Comment at: include/clang/Driver/ToolChain.h:355
+  virtual LangOptions::TrivialAutoVarInitKind
+  GetDefaultTrivialAutoVarInit() const {
+    return LangOptions::TrivialAutoVarInitKind::Uninitialized;
----------------
pcc wrote:
> I wouldn't introduce this function because nothing is overriding it (for now, at least).
I expect to do so for particular targets, so it's only a question of time, and for now it makes internal testing easier (I can build my compiler and override it for testing, before sending that upstream). I'd rather do it now, since it reduces merge conflicts as we kick the tires internally.


================
Comment at: lib/CodeGen/CGDecl.cpp:977
+  constexpr uint32_t SmallValue = 0x000000AA;
+  if (Ty->isIntOrIntVectorTy()) {
+    unsigned BitWidth = llvm::cast<llvm::IntegerType>(
----------------
pcc wrote:
> Do you have test coverage for vectors?
`test/CodeGenCXX/auto-var-init.cpp` has those tests. I was lazy with this patch and hadn't updated the tests yet, in case people asked me to change the values. I've done so in this update.


================
Comment at: lib/CodeGen/CGDecl.cpp:1673
+            llvm::BasicBlock *ContBB = createBasicBlock("vla-init.cont");
+            EmitBlock(LoopBB);
+            llvm::PHINode *Cur =
----------------
pcc wrote:
> I think we need to check that the size is non-zero here. Otherwise we're going to clobber whatever comes after the VLA if its size is zero (I know that C forbids zero-sized VLAs, but I believe we support them as a GNU extension).
Indeed! Thanks for catching that. I thought I'd done it, and it turns out on O0 it wasn't doing anything bad, and neither was it in O2 because the GEPs were marked inbounds so the codegen magically turned into a memset based on size!


================
Comment at: test/CodeGenCXX/trivial-auto-var-init.cpp:5
+
+// PATTERN-NOT: undef
+// ZERO-NOT: undef
----------------
pcc wrote:
> Since this is your only test involving structs and arrays, you probably want to test them beyond the fact that they don't contain undef. I also believe that this is testing for the absence of `undef` before the first label and not the total absence of `undef` in the output.
I was lazy with testing in case the design changed significantly. The updated test explains what I was going for here :)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54604/new/

https://reviews.llvm.org/D54604