[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Looks good with suggestions applied, and with the portability problems in the test fixed. (Maybe just add a `-triple`? Though it would be good to also test `inalloca` and ARM constructor `this` returns and so on.)



================
Comment at: clang/include/clang/Driver/Options.td:3957
+def disable_noundef_args : Flag<["-"], "disable-noundef-args">,
+  HelpText<"Disable emitting noundef attribute in LLVM IR">;
 def load : Separate<["-"], "load">, MetaVarName<"<dsopath>">,
----------------
(since this doesn't affect return values)


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2144
+  bool HasStrictReturn =
+      getLangOpts().CPlusPlus || getLangOpts().OpenCLCPlusPlus;
+  if (TargetDecl) {
----------------
`OpenCLCPlusPlus` should only ever be `true` when `CPlusPlus` is also `true`, so the second half of this `||` should be unnecessary.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2148-2150
+    } else if (const VarDecl *VDecl = dyn_cast<VarDecl>(TargetDecl)) {
+      // Function pointer
+      HasStrictReturn &= !VDecl->isExternC();
----------------
`TargetDecl` (the callee of a function call) should never be a variable. You shouldn't need this check.


================
Comment at: clang/test/CodeGen/attr-noundef.cpp:22
+// CHECK: define void @{{.*}}ret_nocopy{{.*}}(%"struct.check_structs::NoCopy"* noalias sret align 4 %
+// CHECK: define void @{{.*}}pass_nocopy{{.*}}(%"struct.check_structs::NoCopy"* noundef %
+
----------------
I think this test will fail on 32-bit Windows because `e` will be passed `inalloca`.


================
Comment at: clang/test/CodeGen/attr-noundef.cpp:78
+}
+// CHECK: define linkonce_odr void @{{.*}}Object{{.*}}(%"struct.check_this::Object"* noundef %
+// CHECK: define linkonce_odr noundef i32 @{{.*}}Object{{.*}}getData{{.*}}(%"struct.check_this::Object"* noundef %
----------------
I believe this test will fail on ARM, due to constructors implicitly returning `this`. Consider either specifying a target for this test or weakening the `void` to `{{.*}}` here.


================
Comment at: clang/test/CodeGen/attr-noundef.cpp:79
+// CHECK: define linkonce_odr void @{{.*}}Object{{.*}}(%"struct.check_this::Object"* noundef %
+// CHECK: define linkonce_odr noundef i32 @{{.*}}Object{{.*}}getData{{.*}}(%"struct.check_this::Object"* noundef %
+// CHECK: define linkonce_odr noundef %"struct.check_this::Object"* @{{.*}}Object{{.*}}getThis{{.*}}(%"struct.check_this::Object"* noundef %
----------------
I believe this test will fail on Windows, because `getData` and `Object` will appear in the opposite order in the mangling. Consider either specifying a target or matching these names either way around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81678