Merge pull request llvm#623 from AMD-Lightning-Internal/amd/merge/upstream_merge_20250212205251

merge main into amd-staging

Author: ronlieb

bolt/tools/driver/llvm-bolt.cpp

@@ -173,24 +173,14 @@ void boltMode(int argc, char **argv) {
   }
 }
 
-static std::string GetExecutablePath(const char *Argv0) {
-  SmallString<256> ExecutablePath(Argv0);
-  // Do a PATH lookup if Argv0 isn't a valid path.
-  if (!llvm::sys::fs::exists(ExecutablePath))
-    if (llvm::ErrorOr<std::string> P =
-            llvm::sys::findProgramByName(ExecutablePath))
-      ExecutablePath = *P;
-  return std::string(ExecutablePath);
-}
-
 int main(int argc, char **argv) {
   // Print a stack trace if we signal out.
   sys::PrintStackTraceOnErrorSignal(argv[0]);
   PrettyStackTraceProgram X(argc, argv);
 
   llvm_shutdown_obj Y; // Call llvm_shutdown() on exit.
 
-  std::string ToolPath = GetExecutablePath(argv[0]);
+  std::string ToolPath = llvm::sys::fs::getMainExecutable(argv[0], nullptr);
 
   // Initialize targets and assembly printers/parsers.
   llvm::InitializeAllTargetInfos();

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

@@ -294,6 +294,16 @@ ANALYZER_OPTION(
     bool, ShouldUnrollLoops, "unroll-loops",
     "Whether the analysis should try to unroll loops with known bounds.", false)
 
+ANALYZER_OPTION(
+    bool, ShouldAssumeAtLeastOneIteration, "assume-at-least-one-iteration",
+    "Whether the analyzer should always assume at least one iteration in "
+    "loops where the loop condition is opaque (i.e. the analyzer cannot "
+    "determine if it's true or false). Setting this to true eliminates some "
+    "false positives (where e.g. a structure is nonempty, but the analyzer "
+    "does not notice this); but it also eliminates some true positives (e.g. "
+    "cases where a structure can be empty and this causes buggy behavior).",
+    false)
+
 ANALYZER_OPTION(
     bool, ShouldDisplayNotesAsEvents, "notes-as-events",
     "Whether the bug reporter should transparently treat extra note diagnostic "

clang/lib/AST/ByteCode/InterpBuiltin.cpp

@@ -1683,7 +1683,7 @@ static bool interp__builtin_operator_new(InterpState &S, CodePtr OpPC,
   assert(!ElemT);
   // Structs etc.
   const Descriptor *Desc = S.P.createDescriptor(
-      Call, ElemType.getTypePtr(), Descriptor::InlineDescMD,
+      NewCall, ElemType.getTypePtr(), Descriptor::InlineDescMD,
       /*IsConst=*/false, /*IsTemporary=*/false, /*IsMutable=*/false,
       /*Init=*/nullptr);
 

clang/lib/CodeGen/CGStmtOpenMP.cpp

@@ -2922,10 +2922,86 @@ static void emitSimdlenSafelenClause(CodeGenFunction &CGF,
   }
 }
 
+// Check for the presence of an `OMPOrderedDirective`,
+// i.e., `ordered` in `#pragma omp ordered simd`.
+//
+// Consider the following source code:
+// ```
+// __attribute__((noinline)) void omp_simd_loop(float X[ARRAY_SIZE][ARRAY_SIZE])
+// {
+//     for (int r = 1; r < ARRAY_SIZE; ++r) {
+//         for (int c = 1; c < ARRAY_SIZE; ++c) {
+// #pragma omp simd
+//             for (int k = 2; k < ARRAY_SIZE; ++k) {
+// #pragma omp ordered simd
+//                 X[r][k] = X[r][k - 2] + sinf((float)(r / c));
+//             }
+//         }
+//     }
+// }
+// ```
+//
+// Suppose we are in `CodeGenFunction::EmitOMPSimdInit(const OMPLoopDirective
+// &D)`. By examining `D.dump()` we have the following AST containing
+// `OMPOrderedDirective`:
+//
+// ```
+// OMPSimdDirective 0x1c32950
+// `-CapturedStmt 0x1c32028
+//   |-CapturedDecl 0x1c310e8
+//   | |-ForStmt 0x1c31e30
+//   | | |-DeclStmt 0x1c31298
+//   | | | `-VarDecl 0x1c31208  used k 'int' cinit
+//   | | |   `-IntegerLiteral 0x1c31278 'int' 2
+//   | | |-<<<NULL>>>
+//   | | |-BinaryOperator 0x1c31308 'int' '<'
+//   | | | |-ImplicitCastExpr 0x1c312f0 'int' <LValueToRValue>
+//   | | | | `-DeclRefExpr 0x1c312b0 'int' lvalue Var 0x1c31208 'k' 'int'
+//   | | | `-IntegerLiteral 0x1c312d0 'int' 256
+//   | | |-UnaryOperator 0x1c31348 'int' prefix '++'
+//   | | | `-DeclRefExpr 0x1c31328 'int' lvalue Var 0x1c31208 'k' 'int'
+//   | | `-CompoundStmt 0x1c31e18
+//   | |   `-OMPOrderedDirective 0x1c31dd8
+//   | |     |-OMPSimdClause 0x1c31380
+//   | |     `-CapturedStmt 0x1c31cd0
+// ```
+//
+// Note the presence of `OMPOrderedDirective` above:
+// It's (transitively) nested in a `CapturedStmt` representing the pragma
+// annotated compound statement. Thus, we need to consider this nesting and
+// include checking the `getCapturedStmt` in this case.
+static bool hasOrderedDirective(const Stmt *S) {
+  if (isa<OMPOrderedDirective>(S))
+    return true;
+
+  if (const auto *CS = dyn_cast<CapturedStmt>(S))
+    return hasOrderedDirective(CS->getCapturedStmt());
+
+  for (const Stmt *Child : S->children()) {
+    if (Child && hasOrderedDirective(Child))
+      return true;
+  }
+
+  return false;
+}
+
+static void applyConservativeSimdOrderedDirective(const Stmt &AssociatedStmt,
+                                                  LoopInfoStack &LoopStack) {
+  // Check for the presence of an `OMPOrderedDirective`
+  // i.e., `ordered` in `#pragma omp ordered simd`
+  bool HasOrderedDirective = hasOrderedDirective(&AssociatedStmt);
+  // If present then conservatively disable loop vectorization
+  // analogously to how `emitSimdlenSafelenClause` does.
+  if (HasOrderedDirective)
+    LoopStack.setParallel(/*Enable=*/false);
+}
+
 void CodeGenFunction::EmitOMPSimdInit(const OMPLoopDirective &D) {
   // Walk clauses and process safelen/lastprivate.
   LoopStack.setParallel(/*Enable=*/true);
   LoopStack.setVectorizeEnable();
+  const Stmt *AssociatedStmt = D.getAssociatedStmt();
+  applyConservativeSimdOrderedDirective(*AssociatedStmt, LoopStack);
   emitSimdlenSafelenClause(*this, D);
   if (const auto *C = D.getSingleClause<OMPOrderClause>())
     if (C->getKind() == OMPC_ORDER_concurrent)

clang/lib/Driver/ToolChains/AMDGPU.cpp

@@ -740,10 +740,6 @@ amdgpu::dlr::getCommonDeviceLibNames(
   // If --hip-device-lib is not set, add the default bitcode libraries.
   // TODO: There are way too many flags that change this. Do we need to check
   // them all?
-  std::tuple<bool, const SanitizerArgs> GPUSan(
-      DriverArgs.hasFlag(options::OPT_fgpu_sanitize,
-                         options::OPT_fno_gpu_sanitize, true),
-      SanArgs);
   bool DAZ = DriverArgs.hasFlag(
       options::OPT_fgpu_flush_denormals_to_zero,
       options::OPT_fno_gpu_flush_denormals_to_zero,
@@ -760,6 +756,12 @@ amdgpu::dlr::getCommonDeviceLibNames(
       options::OPT_fno_hip_fp32_correctly_rounded_divide_sqrt, true);
   bool Wave64 = toolchains::AMDGPUToolChain::isWave64(DriverArgs, Kind);
 
+  // GPU Sanitizer currently only supports ASan and is enabled through host
+  // ASan.
+  bool GPUSan = DriverArgs.hasFlag(options::OPT_fgpu_sanitize,
+                                   options::OPT_fno_gpu_sanitize, true) &&
+                SanArgs.needsAsanRt();
+
   return RocmInstallation.getCommonBitcodeLibs(
       DriverArgs, LibDeviceFile, Wave64, DAZ, FiniteOnly, UnsafeMathOpt,
       FastRelaxedMath, CorrectSqrt, ABIVer, GPUSan, isOpenMP);
@@ -1008,13 +1010,7 @@ void ROCMToolChain::addClangTargetOptions(
                                                 ABIVer, noGPULib))
     return;
 
-  std::tuple<bool, const SanitizerArgs> GPUSan(
-      DriverArgs.hasFlag(options::OPT_fgpu_sanitize,
-                         options::OPT_fno_gpu_sanitize, true),
-      getSanitizerArgs((DriverArgs)));
-
   bool Wave64 = isWave64(DriverArgs, Kind);
-
   // TODO: There are way too many flags that change this. Do we need to check
   // them all?
   bool DAZ = DriverArgs.hasArg(options::OPT_cl_denorms_are_zero) ||
@@ -1027,6 +1023,12 @@ void ROCMToolChain::addClangTargetOptions(
   bool CorrectSqrt =
       DriverArgs.hasArg(options::OPT_cl_fp32_correctly_rounded_divide_sqrt);
 
+  // GPU Sanitizer currently only supports ASan and is enabled through host
+  // ASan.
+  bool GPUSan = DriverArgs.hasFlag(options::OPT_fgpu_sanitize,
+                                   options::OPT_fno_gpu_sanitize, true) &&
+                getSanitizerArgs(DriverArgs).needsAsanRt();
+
   // Add the OpenCL specific bitcode library.
   llvm::SmallVector<BitCodeLibraryInfo, 12> BCLibs;
   BCLibs.emplace_back(RocmInstallation->getOpenCLPath().str());
@@ -1070,24 +1072,18 @@ llvm::SmallVector<ToolChain::BitCodeLibraryInfo, 12>
 RocmInstallationDetector::getCommonBitcodeLibs(
     const llvm::opt::ArgList &DriverArgs, StringRef LibDeviceFile, bool Wave64,
     bool DAZ, bool FiniteOnly, bool UnsafeMathOpt, bool FastRelaxedMath,
-    bool CorrectSqrt, DeviceLibABIVersion ABIVer,
-    const std::tuple<bool, const SanitizerArgs> &GPUSan,
-    bool isOpenMP = false) const {
+    bool CorrectSqrt, DeviceLibABIVersion ABIVer, bool GPUSan,
+    bool isOpenMP) const {
   llvm::SmallVector<ToolChain::BitCodeLibraryInfo, 12> BCLibs;
 
-  auto GPUSanEnabled = [GPUSan]() {
-    return std::get<bool>(GPUSan) &&
-           std::get<const SanitizerArgs>(GPUSan).needsAsanRt();
-  };
   auto AddBCLib = [&](ToolChain::BitCodeLibraryInfo BCLib,
                       bool Internalize = true) {
     BCLib.ShouldInternalize = Internalize;
     BCLibs.push_back(BCLib);
   };
   auto AddSanBCLibs = [&]() {
-    if (GPUSanEnabled()) {
+    if (GPUSan)
       AddBCLib(getAsanRTLPath(), false);
-    }
   };
 
   AddSanBCLibs();
@@ -1100,7 +1096,7 @@ RocmInstallationDetector::getCommonBitcodeLibs(
   // __BUILD_MATH_BUILTINS_LIB__ turning static libm functions to extern.
   if (!isOpenMP)
     AddBCLib(getOCKLPath());
-  else if (GPUSanEnabled() && isOpenMP)
+  else if (GPUSan && isOpenMP)
     AddBCLib(getOCKLPath(), false);
   AddBCLib(getDenormalsAreZeroPath(DAZ));
   AddBCLib(getUnsafeMathPath(UnsafeMathOpt || FastRelaxedMath));
@@ -1140,6 +1136,7 @@ bool AMDGPUToolChain::shouldSkipSanitizeOption(
   if (TargetID.empty())
     return false;
   Option O = A->getOption();
+
   if (!O.matches(options::OPT_fsanitize_EQ))
     return false;
 

clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp

@@ -458,11 +458,18 @@ llvm::opt::DerivedArgList *AMDGPUOpenMPToolChain::TranslateArgs(
 
   DerivedArgList *DAL =
       HostTC.TranslateArgs(Args, BoundArch, DeviceOffloadKind);
-  if (!DAL || Args.hasArg(options::OPT_fsanitize_EQ))
+
+  if (!DAL)
     DAL = new DerivedArgList(Args.getBaseArgs());
 
   const OptTable &Opts = getDriver().getOpts();
 
+  // Skip sanitize options passed from the HostTC. Claim them early.
+  // The decision to sanitize device code is computed only by
+  // 'shouldSkipSanitizeOption'.
+  if (DAL->hasArg(options::OPT_fsanitize_EQ))
+    DAL->claimAllArgs(options::OPT_fsanitize_EQ);
+
   for (Arg *A : Args)
     if (!shouldSkipSanitizeOption(*this, Args, BoundArch, A) &&
         !llvm::is_contained(*DAL, A))

clang/lib/Driver/ToolChains/ROCm.h

@@ -178,7 +178,7 @@ class RocmInstallationDetector {
       const llvm::opt::ArgList &DriverArgs, StringRef LibDeviceFile,
       bool Wave64, bool DAZ, bool FiniteOnly, bool UnsafeMathOpt,
       bool FastRelaxedMath, bool CorrectSqrt, DeviceLibABIVersion ABIVer,
-      const std::tuple<bool, const SanitizerArgs> &GPUSan, bool isOpenMP) const;
+      bool GPUSan, bool isOpenMP) const;
 
   /// Check file paths of default bitcode libraries common to AMDGPU based
   /// toolchains. \returns false if there are invalid or missing files.

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

@@ -2813,13 +2813,24 @@ void ExprEngine::processBranch(
     if (StTrue && StFalse)
       assert(!isa<ObjCForCollectionStmt>(Condition));
 
+    // We want to ensure consistent behavior between `eagerly-assume=false`,
+    // when the state split is always performed by the `assumeCondition()`
+    // call within this function and `eagerly-assume=true` (the default), when
+    // some conditions (comparison operators, unary negation) can trigger a
+    // state split before this callback. There are some contrived corner cases
+    // that behave differently with and without `eagerly-assume`, but I don't
+    // know about an example that could plausibly appear in "real" code.
+    bool BothFeasible =
+        (StTrue && StFalse) ||
+        didEagerlyAssumeBifurcateAt(PrevState, dyn_cast<Expr>(Condition));
+
     if (StTrue) {
-      // If we are processing a loop condition where two iterations have
-      // already been completed and the false branch is also feasible, then
-      // don't assume a third iteration because it is a redundant execution
-      // path (unlikely to be different from earlier loop exits) and can cause
-      // false positives if e.g. the loop iterates over a two-element structure
-      // with an opaque condition.
+      // In a loop, if both branches are feasible (i.e. the analyzer doesn't
+      // understand the loop condition) and two iterations have already been
+      // completed, then don't assume a third iteration because it is a
+      // redundant execution path (unlikely to be different from earlier loop
+      // exits) and can cause false positives if e.g. the loop iterates over a
+      // two-element structure with an opaque condition.
       //
       // The iteration count "2" is hardcoded because it's the natural limit:
       // * the fact that the programmer wrote a loop (and not just an `if`)
@@ -2830,10 +2841,7 @@ void ExprEngine::processBranch(
       //   two iterations". (This pattern is common in FFMPEG and appears in
       //   many other projects as well.)
       bool CompletedTwoIterations = IterationsCompletedInLoop.value_or(0) >= 2;
-      bool FalseAlsoFeasible =
-          StFalse ||
-          didEagerlyAssumeBifurcateAt(PrevState, dyn_cast<Expr>(Condition));
-      bool SkipTrueBranch = CompletedTwoIterations && FalseAlsoFeasible;
+      bool SkipTrueBranch = BothFeasible && CompletedTwoIterations;
 
       // FIXME: This "don't assume third iteration" heuristic partially
       // conflicts with the widen-loop analysis option (which is off by
@@ -2843,8 +2851,25 @@ void ExprEngine::processBranch(
         Builder.generateNode(StTrue, true, PredN);
     }
 
-    if (StFalse)
-      Builder.generateNode(StFalse, false, PredN);
+    if (StFalse) {
+      // In a loop, if both branches are feasible (i.e. the analyzer doesn't
+      // understand the loop condition), we are before the first iteration and
+      // the analyzer option `assume-at-least-one-iteration` is set to `true`,
+      // then avoid creating the execution path where the loop is skipped.
+      //
+      // In some situations this "loop is skipped" execution path is an
+      // important corner case that may evade the notice of the developer and
+      // hide significant bugs -- however, there are also many situations where
+      // it's guaranteed that at least one iteration will happen (e.g. some
+      // data structure is always nonempty), but the analyzer cannot realize
+      // this and will produce false positives when it assumes that the loop is
+      // skipped.
+      bool BeforeFirstIteration = IterationsCompletedInLoop == std::optional{0};
+      bool SkipFalseBranch = BothFeasible && BeforeFirstIteration &&
+                             AMgr.options.ShouldAssumeAtLeastOneIteration;
+      if (!SkipFalseBranch)
+        Builder.generateNode(StFalse, false, PredN);
+    }
   }
   currBldrCtx = nullptr;
 }

clang/test/AST/ByteCode/new-delete.cpp

@@ -840,10 +840,17 @@ template <typename T>
 struct SS {
     constexpr SS(unsigned long long N)
     : data(nullptr){
-        data = alloc.allocate(N);  // #call
+        data = alloc.allocate(N);
         for(std::size_t i = 0; i < N; i ++)
-            std::construct_at<T>(data + i, i); // #construct_call
+            std::construct_at<T>(data + i, i);
     }
+
+    constexpr SS()
+    : data(nullptr){
+        data = alloc.allocate(1);
+        std::construct_at<T>(data);
+    }
+
     constexpr T operator[](std::size_t i) const {
       return data[i];
     }
@@ -855,6 +862,7 @@ struct SS {
     T* data;
 };
 constexpr unsigned short ssmall = SS<unsigned short>(100)[42];
+constexpr auto Ss = SS<S>()[0];
 
 
 

clang/test/Analysis/analyzer-config.c

@@ -10,6 +10,7 @@
 // CHECK-NEXT: alpha.cplusplus.STLAlgorithmModeling:AggressiveStdFindModeling = false
 // CHECK-NEXT: alpha.osx.cocoa.DirectIvarAssignment:AnnotatedFunctions = false
 // CHECK-NEXT: apply-fixits = false
+// CHECK-NEXT: assume-at-least-one-iteration = false
 // CHECK-NEXT: assume-controlled-environment = false
 // CHECK-NEXT: avoid-suppressing-null-argument-paths = false
 // CHECK-NEXT: c++-allocator-inlining = true