[msan][CodeGen] Set noundef for C return value

[vitalybuka]

Summary

Msan needs noundef consistency between interface and implementation. If
we call C++ from C we can have noundef on C++ side, and no noundef on
caller C side, noundef implementation will not set TLS for return value,
no noundef caller will expect it. Then we have false reports in msan.

The workaround could be set TLS to zero even for noundef return values.
However if we do that always it will increase binary size by about 10%.
If we do that selectively we need to handle "address is taken"
functions, any non local functions, and probably all function which have
musttail callers. Which is still a lot.

The existing implementation of HasStrictReturn refers to C standard as
the reason not enforcing noundef. I believe it applies only to the case
when return statement is omitted. Testing on Google codebase I never see
such cases, however I've see tens of cases where C code returns actual
uninitialized variables, but we ignore that it because of "omitted
return" case.

So this patch will:

1. fix false-positives with TLS missmatch.
2. detect bugs returning uninitialized variables for C as well.
3. report "omitted return" cases stricter than C, which is already a warning and very likely a bug in a code anyway.

[vitalybuka]

new line

[kda]

Is there a risk of this being too strict for standard C?

Should there be a test case for our specific problem? (C calling C++)

[vitalybuka]

In D139296#3972142, @kda wrote:

Is there a risk of this being too strict for standard C?

Should there be a test case for our specific problem? (C calling C++)

Probably not. It's cross module mismatch. To reproduce we need two module and one of them will do about the same as the current msan-param-retval.c.

[kda]

LGTM

[aeubanks]

clang/lib/CodeGen/CGCall.cpp

1827–1828

should this be dropped? and maybe also move up SanitizerKind::Return?

[vitalybuka]

remove redundant check

[vitalybuka]

clang/lib/CodeGen/CGCall.cpp

1827–1828

Documentation for SanitizerKind::Return explicitly states that it's C++ check.
So I will leave it as is.

clang/lib/CodeGen/CGCall.cpp

if (const RecordType *RT =
     if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RT->getDecl()))
       return ClassDecl->hasTrivialDestructor();
   }
   return ReturnType.isTriviallyCopyableType(Context);
 }
 
 static bool HasStrictReturn(const CodeGenModule &Module, QualType RetTy,
                             const Decl *TargetDecl) {
+  // As-is msan can not tolerate noundef mismatch between caller and
+  // implementation. Mismatch is possible for e.g. indirect calls from C-caller
+  // into C++. Such mismatches lead to confusing false reports. To avoid
+  // expensive workaround on msan we enforce initialization event in uncommon
+  // cases where it's allowed.
+  if (Module.getLangOpts().Sanitize.has(SanitizerKind::Memory))
+    return true;
   // C++ explicitly makes returning undefined values UB. C's rule only applies
   // to used values, so we never mark them noundef for now.
   if (!Module.getLangOpts().CPlusPlus)
     return false;
   if (TargetDecl) {
     if (const FunctionDecl *FDecl = dyn_cast<FunctionDecl>(TargetDecl)) {
       if (FDecl->isExternC())
         return false;
     } else if (const VarDecl *VDecl = dyn_cast<VarDecl>(TargetDecl)) {
       // Function pointer.
       if (VDecl->isExternC())
         return false;
     }
   }
 
   // We don't want to be too aggressive with the return checking, unless
   // it's explicit in the code opts or we're using an appropriate sanitizer.
   // Try to respect what the programmer intended.
   return Module.getCodeGenOpts().StrictReturn ||
          !Module.MayDropFunctionReturn(Module.getContext(), RetTy) ||
-         Module.getLangOpts().Sanitize.has(SanitizerKind::Memory) ||
          Module.getLangOpts().Sanitize.has(SanitizerKind::Return);
 }
 
 void CodeGenModule::getDefaultFunctionAttributes(StringRef Name,
                                                  bool HasOptnone,
                                                  bool AttrOnCallSite,
                                                llvm::AttrBuilder &FuncAttrs) {
   // OptimizeNoneAttr takes precedence over -Os or -Oz. No warning needed.
   if (!HasOptnone) {

clang/test/CodeGen/cleanup-destslot-simple.c

 // CHECK-MSAN:       7:
 // CHECK-MSAN-NEXT:    [[TMP8:%.*]] = load i32, ptr [[P_0_P_0_P_0_P_0_]], align 4, !dbg [[DBG20]], !tbaa [[TBAA11]]
 // CHECK-MSAN-NEXT:    [[TMP9:%.*]] = ptrtoint ptr [[P_0_P_0_P_0_P_0_]] to i64, !dbg [[DBG20]]
 // CHECK-MSAN-NEXT:    [[TMP10:%.*]] = xor i64 [[TMP9]], 87960930222080, !dbg [[DBG20]]
 // CHECK-MSAN-NEXT:    [[TMP11:%.*]] = inttoptr i64 [[TMP10]] to ptr, !dbg [[DBG20]]
 // CHECK-MSAN-NEXT:    [[_MSLD1:%.*]] = load i32, ptr [[TMP11]], align 4, !dbg [[DBG20]]
 // CHECK-MSAN-NEXT:    call void @llvm.lifetime.end.p0(i64 8, ptr nonnull [[P]]), !dbg [[DBG22:![0-9]+]]
 // CHECK-MSAN-NEXT:    call void @llvm.lifetime.end.p0(i64 4, ptr nonnull [[X]]) #[[ATTR2]], !dbg [[DBG22]]
-// CHECK-MSAN-NEXT:    store i32 [[_MSLD1]], ptr @__msan_retval_tls, align 8, !dbg [[DBG23:![0-9]+]]
+// CHECK-MSAN-NEXT:    [[_MSCMP2_NOT:%.*]] = icmp eq i32 [[_MSLD1]], 0, !dbg [[DBG23:![0-9]+]]
+// CHECK-MSAN-NEXT:    br i1 [[_MSCMP2_NOT]], label [[TMP13:%.*]], label [[TMP12:%.*]], !dbg [[DBG23]], !prof [[PROF21]]
+// CHECK-MSAN:       12:
+// CHECK-MSAN-NEXT:    call void @__msan_warning_noreturn() #[[ATTR3]], !dbg [[DBG23]]
+// CHECK-MSAN-NEXT:    unreachable, !dbg [[DBG23]]
+// CHECK-MSAN:       13:
 // CHECK-MSAN-NEXT:    ret i32 [[TMP8]], !dbg [[DBG23]]
 //
 // CHECK-KMSAN-LABEL: @test(
 // CHECK-KMSAN-NEXT:  entry:
 // CHECK-KMSAN-NEXT:    [[TMP0:%.*]] = call ptr @__msan_get_context_state() #[[ATTR2:[0-9]+]]
 // CHECK-KMSAN-NEXT:    [[X:%.*]] = alloca i32, align 4
 // CHECK-KMSAN-NEXT:    [[P:%.*]] = alloca ptr, align 8
 // CHECK-KMSAN-NEXT:    call void @llvm.lifetime.start.p0(i64 4, ptr nonnull [[X]]) #[[ATTR2]], !dbg [[DBG9:![0-9]+]]

clang/test/CodeGen/msan-param-retval.c

 // NOUNDEF_ONLY: @__msan_param_tls
 // EAGER-NOT:    @__msan_param_tls
 // CHECK: }
 
 int foo() {
   return 1;
 }
 
-// CHECK: define dso_local i32 @foo() #0 {
-// CHECK:   @__msan_retval_tls
+// CLEAN:   define dso_local i32 @foo() #0 {
+// NOUNDEF: define dso_local noundef i32 @foo() #0 {
+// CLEAN:        @__msan_retval_tls
+// NOUNDEF_ONLY: @__msan_retval_tls
+// EAGER-NOT:    @__msan_retval_tls
 // CHECK: }
 
 int noret() {
 }
 
-// CHECK: define dso_local i32 @noret() #0 {
+// CLEAN:   define dso_local i32 @noret() #0 {   
+// NOUNDEF: define dso_local noundef i32 @noret() #0 {
 // CHECK:   %retval = alloca
-// CHECK: }
+// CLEAN:        @__msan_retval_tls
+// NOUNDEF_ONLY: @__msan_retval_tls
+// EAGER-NOT:    @__msan_retval_tls
+// CHECK: }