Add -fsanitize-address-param-retval to clang.

[kda]

Summary

With the introduction of this flag, it is no longer necessary to enable noundef analysis with 4 separate flags.
(-Xclang -enable-noundef-analysis -mllvm -msan-eager-checks=1).
This change only covers the introduction into the compiler.

This is a follow up to: https://reviews.llvm.org/D116855

[vitalybuka]

clang/include/clang/Driver/Options.td

1677

Maybe "Detect initialized parameters and return values."? and above

clang/lib/CodeGen/CGCall.cpp

2384 ↗

(On Diff #397426)

@eugenis Would be better to force CodeGenOpts.EnableNoundefAttrs if SanitizeMemoryParamRetval is provided?

2386 ↗

(On Diff #397426)

Without this change "-enable-noundef-analysis with -fsanitize-memory-param-retval" can be used.
So no matter what we decide about EnableNoundefAttrs, let's move this change and tests below into a separate incremental patch.

[vitalybuka]

clang/lib/CodeGen/CGCall.cpp

2384 ↗

(On Diff #397426)

@eugenis Would be better to force CodeGenOpts.EnableNoundefAttrs if SanitizeMemoryParamRetval is provided?

To clarify, checking SanitizeMemoryParamRetval here as-is is LGTM, unless @eugenis or someone else thinks EnableNoundefAttrs reset is better.

[kda]

updated help text and dropped implementation

[kda]

clang/include/clang/Driver/Options.td

1677

I think you meant 'uninitialized'. Otherwise changed as requested.

[kda]

trying again

[eugenis]

clang/lib/CodeGen/CGCall.cpp

2384 ↗

(On Diff #397426)

I don't think this is right at all. An argument being noundef has nothing to do at all with memory sanitization. It affects optimization, too. SanitizeMemoryParamRetval should only affect what MemorySanitizerPass does with noundef attributes.

[kda]

clang/lib/CodeGen/CGCall.cpp

2384 ↗

(On Diff #397426)

Is the problem the form of the new code?

• instead of introducing a single new flag to flip 4 others, should there be 1 flag to pick up the CodeGen changes, and another for the Sanitizer? (Is 2 flags better than 4?)
• another option is have the changes propagate in the other direction: use the flag (-fsanitize-memory-param-retval), to passes along '-mllvm -enable_noundef_analysis=1' to CodeGen (via SanitizerArgs.addArgs).

OR is there a problem forcing EnableNoundefAttrs based on an "unrelated" flag?

• then leave existing code, just don't do anything fancy to change EnableNoundefAttrs.

[vitalybuka]

we probably should to move clang parts from D116634 into this patch?

clang/include/clang/Basic/CodeGenOptions.def

234

Same comment here?

clang/lib/CodeGen/CGCall.cpp

2384 ↗

(On Diff #397426)

I don't think this is right at all. An argument being noundef has nothing to do at all with memory sanitization. It affects optimization, too. SanitizeMemoryParamRetval should only affect what MemorySanitizerPass does with noundef attributes.

What is the point of running SanitizeMemoryParamRetval==1 && EnableNoundefAttrs==0?
Are we going to show warning or error and ask to set thee redundant options? Then why not just automate this for the user?
Also please note that HasStrictReturn already check SanitizerKind::Memory and adjust NoUndef attributes.

On the other hand, if EnableNoundefAttrs is going to be true by default in future, this conversation is not important, we can have two flags.

2384 ↗

(On Diff #397426)

Is the problem the form of the new code?

• instead of introducing a single new flag to flip 4 others, should there be 1 flag to pick up the CodeGen changes, and another for the Sanitizer? (Is 2 flags better than 4?)
• another option is have the changes propagate in the other direction: use the flag (-fsanitize-memory-param-retval), to passes along '-mllvm -enable_noundef_analysis=1' to CodeGen (via SanitizerArgs.addArgs).

OR is there a problem forcing EnableNoundefAttrs based on an "unrelated" flag?

• then leave existing code, just don't do anything fancy to change EnableNoundefAttrs.

I believe @eugenis point that user should proved both -fsanitize-memory-param-retval and -enable-noundef-analysis to clang (we don't need -mllvm for -enable-noundef-analysis already).

[vitalybuka]

This comment was removed by vitalybuka.

[eugenis]

clang/lib/CodeGen/CGCall.cpp

2384 ↗

(On Diff #397426)

Sorry, I think I misunderstood the code. It makes sense for -fsanitize-memory-param-retval to implicitly enable noundef attrs. I agree with Vitaly that it is better to change the value of EnableNoundefAttrs, to avoid the risk of enabling only part of that feature (now or with some future change).

[MaskRay]

This needs a clang/test/Driver test to show that %clang -fsanitize-memory-param-retval translates to CC1 -fsanitize-memory-param-retval.

clang/include/clang/Driver/Options.td

1677

--help messages don't have periods.

1677

Does --help message render clearly?

[kda]

clang/include/clang/Driver/Options.td

1677

Indeed:

-fsanitize-memory-param-retval
                        EnableDetect uninitialized parameters and return values

[kda]

adjust help text

[kda]

clang/include/clang/Driver/Options.td

1677

Maybe not...

[kda]

refined rendering of help text

clang/include/clang/Driver/Options.td

1677

much better

-fsanitize-memory-param-retval
                        Enable detection of uninitialized parameters and return values

[kda]

A few improvements.

• Add -fsanitize-address-param-retval to clang.
• updated flag help text and dropped use of flag (to be picked up later).
• adjust help text
• refined rendering of help text
• add code generation
• introduce member CodeGenModule::NoundefAnalysisEnabled

[kda]

PTAL

[kda]

In D116633#3226228, @MaskRay wrote:

This needs a clang/test/Driver test to show that %clang -fsanitize-memory-param-retval translates to CC1 -fsanitize-memory-param-retval.

Could you point me to an example?

[MaskRay]

In D116633#3236352, @kda wrote:

In D116633#3226228, @MaskRay wrote:

This needs a clang/test/Driver test to show that %clang -fsanitize-memory-param-retval translates to CC1 -fsanitize-memory-param-retval.

Could you point me to an example?

test/Driver/fpatchable-function-entry.c test/Driver/fbinutils-version.c :)

[kda]

add test to validate that flag does not conflict with enable-noundef-analysis

[kda]

This needs a clang/test/Driver test to show that %clang -fsanitize-memory-param-retval translates to CC1 -fsanitize-memory-param-retval.

Could you point me to an example?

test/Driver/fpatchable-function-entry.c test/Driver/fbinutils-version.c :)

I added some conflict tests here.
I added the CC1 tests to: https://reviews.llvm.org/D116701

[kda]

Drop attribute changes.

[kda]

enable eager-checks with -fsanitize-memory-param-retval

[kda]

PTAL

[vitalybuka]

LGTM with few nits

clang/lib/CodeGen/BackendUtil.cpp

362 ↗

(On Diff #399826)

we use implicit cast mostly, e.g. addAddressSanitizerPasses

clang/test/CodeGen/param-retval-eager-checks.c

1 ↗

(On Diff #399826)

Would you like to remove "| \" ?
80 char limit is not enforced on tests, multi line RUN: is even harder to read then long line

1 ↗

(On Diff #399826)

This dir contains tests for a lot of different componets
can you please add msan in name

e.g. "msan-param-retval.c"

5 ↗

(On Diff #399826)

we probably don't need mllvm test here

[kda]

rename file

[kda]

clang/lib/CodeGen/BackendUtil.cpp

362 ↗

(On Diff #399826)

I agree, AND the compiler complains of needing a static_cast<bool>.
Of course, below (line 1167) where the constructor is called explicitly, it works fine.
I think this is a case of 2 implicit casts and hence the compiler balks.

There are 2 other possibilities aside from the static_cast<bool>:

• assign to a bool, then use the bool
• construct the options on the stack (like line 1167)

I found my solution to be the most direct, clear and simple.

clang/test/CodeGen/param-retval-eager-checks.c

1 ↗

(On Diff #399826)

Would you like to remove "| \" ?
80 char limit is not enforced on tests, multi line RUN: is even harder to read then long line

No. I broke the lines there as it makes it easier to see the differences, as the flags and the FileCheck are aligned. (It wasn't about 80 characters.)

5 ↗

(On Diff #399826)

No, not needed.
And I would like to keep it, as it shows the equivalency between the flags.

[vitalybuka]

clang/test/Driver/fsanitize-memory-param-retval.c

9–10

I guess this copied by mistake from example. This is essentially the same as line 2.

[kda]

clang/test/Driver/fsanitize-memory-param-retval.c

9–10

Should I drop it?
I was trying to copy all the subtleties found in other tests.
The next test shows the opposite.

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(SanitizeAddressGlobalsDeadStripping, 1, 0) ///< Enable linker dead stripping
                                                       ///< of globals in AddressSanitizer
 CODEGENOPT(SanitizeAddressUseOdrIndicator, 1, 0) ///< Enable ODR indicator globals
 CODEGENOPT(SanitizeMemoryTrackOrigins, 2, 0) ///< Enable tracking origins in
                                              ///< MemorySanitizer
 ENUM_CODEGENOPT(SanitizeAddressDtor, llvm::AsanDtorKind, 2,
                 llvm::AsanDtorKind::Global)  ///< Set how ASan global
                                              ///< destructors are emitted.
+CODEGENOPT(SanitizeMemoryParamRetval, 1, 0) ///< Enable detection of uninitialized
+                                            ///< parameters and return values
+                                            ///< in MemorySanitizer
 CODEGENOPT(SanitizeMemoryUseAfterDtor, 1, 0) ///< Enable use-after-delete detection
                                              ///< in MemorySanitizer
 CODEGENOPT(SanitizeCfiCrossDso, 1, 0) ///< Enable cross-dso support in CFI.
 CODEGENOPT(SanitizeMinimalRuntime, 1, 0) ///< Use "_minimal" sanitizer runtime for
                                          ///< diagnostics.
 CODEGENOPT(SanitizeCfiICallGeneralizePointers, 1, 0) ///< Generalize pointer types in
                                                      ///< CFI icall function signatures
 CODEGENOPT(SanitizeCfiCanonicalJumpTables, 1, 0) ///< Make jump table symbols canonical

clang/include/clang/Driver/Options.td

def sanitize_address_destructor_EQ
     : Joined<["-"], "fsanitize-address-destructor=">,
       Flags<[CC1Option]>,
       HelpText<"Set destructor type used in ASan instrumentation">,
       Group<f_clang_Group>,
       Values<"none,global">,
       NormalizedValuesScope<"llvm::AsanDtorKind">,
       NormalizedValues<["None", "Global"]>,
       MarshallingInfoEnum<CodeGenOpts<"SanitizeAddressDtor">, "Global">;
+defm sanitize_memory_param_retval
+  : BoolOption<"f", "sanitize-memory-param-retval",
+      CodeGenOpts<"SanitizeMemoryParamRetval">,
+      DefaultFalse,
+      PosFlag<SetTrue, [CC1Option], "Enable">, NegFlag<SetFalse, [], "Disable">,
+      BothFlags<[], " detection of uninitialized parameters and return values">>,
+    Group<f_clang_Group>;
 // Note: This flag was introduced when it was necessary to distinguish between
 //       ABI for correct codegen.  This is no longer needed, but the flag is
 //       not removed since targeting either ABI will behave the same.
 //       This way we cause no disturbance to existing scripts & code, and if we
 //       want to use this flag in the future we will cause no disturbance then
 //       either.
 def fsanitize_hwaddress_abi_EQ
     : Joined<["-"], "fsanitize-hwaddress-abi=">,

clang/include/clang/Driver/SanitizerArgs.h

class SanitizerArgs {
 
   std::vector<std::string> UserIgnorelistFiles;
   std::vector<std::string> SystemIgnorelistFiles;
   std::vector<std::string> CoverageAllowlistFiles;
   std::vector<std::string> CoverageIgnorelistFiles;
   int CoverageFeatures = 0;
   int MsanTrackOrigins = 0;
   bool MsanUseAfterDtor = true;
+  bool MsanParamRetval = false;
   bool CfiCrossDso = false;
   bool CfiICallGeneralizePointers = false;
   bool CfiCanonicalJumpTables = false;
   int AsanFieldPadding = 0;
   bool SharedRuntime = false;
   bool AsanUseAfterScope = true;
   bool AsanPoisonCustomArrayCookie = false;
   bool AsanGlobalsDeadStripping = false;

clang/lib/Driver/SanitizerArgs.cpp

if (Arg *A =
                 << A->getAsString(Args) << S;
         }
       }
     }
     MsanUseAfterDtor =
         Args.hasFlag(options::OPT_fsanitize_memory_use_after_dtor,
                      options::OPT_fno_sanitize_memory_use_after_dtor,
                      MsanUseAfterDtor);
+    MsanParamRetval = Args.hasFlag(
+        options::OPT_fsanitize_memory_param_retval,
+        options::OPT_fno_sanitize_memory_param_retval, MsanParamRetval);
     NeedPIE |= !(TC.getTriple().isOSLinux() &&
                  TC.getTriple().getArch() == llvm::Triple::x86_64);
   } else {
     MsanUseAfterDtor = false;
+    MsanParamRetval = false;
   }
 
   if (AllAddedKinds & SanitizerKind::Thread) {
     TsanMemoryAccess = Args.hasFlag(
         options::OPT_fsanitize_thread_memory_access,
         options::OPT_fno_sanitize_thread_memory_access, TsanMemoryAccess);
     TsanFuncEntryExit = Args.hasFlag(
         options::OPT_fsanitize_thread_func_entry_exit,
void SanitizerArgs::addArgs(const ToolChain &TC, const llvm::opt::ArgList &Args,
 
   if (MsanTrackOrigins)
     CmdArgs.push_back(Args.MakeArgString("-fsanitize-memory-track-origins=" +
                                          Twine(MsanTrackOrigins)));
 
   if (MsanUseAfterDtor)
     CmdArgs.push_back("-fsanitize-memory-use-after-dtor");
 
+  if (MsanParamRetval)
+    CmdArgs.push_back("-fsanitize-memory-param-retval");
+
   // FIXME: Pass these parameters as function attributes, not as -llvm flags.
   if (!TsanMemoryAccess) {
     CmdArgs.push_back("-mllvm");
     CmdArgs.push_back("-tsan-instrument-memory-accesses=0");
     CmdArgs.push_back("-mllvm");
     CmdArgs.push_back("-tsan-instrument-memintrinsics=0");
   }
   if (!TsanFuncEntryExit) {

clang/test/Driver/fsanitize-memory-param-retval.c

+// RUN: %clang -target i386-gnu-linux %s -fsanitize=memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-linux-gnu %s -fsanitize=memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target aarch64-linux-gnu %s -fsanitize=memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-linux-gnu %s -fsanitize=memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-linux-gnu %s -fsanitize=memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
+// CHECK: "-fsanitize-memory-param-retval"
+
+// RUN: %clang -target aarch64-linux-gnu -fsyntax-only %s -fsanitize=memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck --check-prefix=11 %s
+// 11: "-fsanitize-memory-param-retval"
+
+// RUN: not %clang -target x86_64-linux-gnu -fsyntax-only %s -fsanitize=memory -fsanitize-memory-param-retval=1 2>&1 | FileCheck --check-prefix=EXCESS %s
+// EXCESS: error: unknown argument: '-fsanitize-memory-param-retval=