[flang][OpenMP] Fix warning due to uninitialized pointer dereference during atomic update lowering

[NimishMishra]

Summary

This patch places a guard around to prevent nullptr deference during atomic update lowering

[peixin]

flang/lib/Lower/OpenMP.cpp

1129–1130

Can you move the code in line 1130-1138 to line 1164?

1130

Is it better to move this assert outside in line 1164?

[kiranchandramohan]

You can submit it after addressing @peixin's comment. If there are issues then revert D125668.

flang/lib/Lower/OpenMP.cpp

1129–1130

+1

[kazu]

Thank you for the patch. I am wondering if it's better to completely remove the if statements. Please see the inline comments. Thanks again!

flang/lib/Lower/OpenMP.cpp

1129–1130

If you expect updateSymbol to be always set, could we do something like this? We can catch errors earlier this way. Also, if statements that are always taken should be assert instead.

auto varDesignator =
    std::get_if<Fortran::common::Indirection<Fortran::parser::Designator>>(
        &assignmentStmtVariable.u);
assert(varDesignator && "varDesignator must have a value.");
const auto *name = getDesignatorNameIfDataRef(varDesignator->value());
assert(name && name->symbol &&
       "No symbol attached to atomic update variable.");
const Fortran::semantics::Symbol *updateSymbol = name->symbol;

[kiranchandramohan]

flang/lib/Lower/OpenMP.cpp

1129–1130

That is a nice suggestion.

[NimishMishra]

Addressed comments.

[kazu]

Thank you for updating the patch. Please see the inline comment.

flang/lib/Lower/OpenMP.cpp

1156–1164

If you are moving this block of code, you might as well do:

const Fortran::semantics::Symbol *updateSymbol = name->symbol;
converter.bindSymbol(*updateSymbol, val);

or drop the intermediate variable altogether:

converter.bindSymbol(*(name->symbol), val);

I would avoid an unused value like = nullptr in the straight-line code. Some compilers might even issue a warning like "set but unused" or something.

[NimishMishra]

Addressed comment.

[kazu]

LGTM. Thank you for updating the patch!

flang/lib/Lower/OpenMP.cpp

static void genOmpAtomicUpdateStatement(
     const Fortran::parser::OmpAtomicClauseList *leftHandClauseList,
     const Fortran::parser::OmpAtomicClauseList *rightHandClauseList) {
   // Generate `omp.atomic.update` operation for atomic assignment statements
   auto &firOpBuilder = converter.getFirOpBuilder();
   auto currentLocation = converter.getCurrentLocation();
   Fortran::lower::StatementContext stmtCtx;
 
   mlir::Value address = fir::getBase(converter.genExprAddr(
       *Fortran::semantics::GetExpr(assignmentStmtVariable), stmtCtx));
-  const Fortran::semantics::Symbol *updateSymbol;
-  if (auto varDesignator = std::get_if<
-          Fortran::common::Indirection<Fortran::parser::Designator>>(
-          &assignmentStmtVariable.u)) {
-    if (const auto *name = getDesignatorNameIfDataRef(varDesignator->value())) {
-      updateSymbol = name->symbol;
-    }
-  }
   // If no hint clause is specified, the effect is as if
   // hint(omp_sync_hint_none) had been specified.
   mlir::IntegerAttr hint = nullptr;
   mlir::omp::ClauseMemoryOrderKindAttr memory_order = nullptr;
   if (leftHandClauseList)
     genOmpAtomicHintAndMemoryOrderClauses(converter, *leftHandClauseList, hint,
                                           memory_order);
   if (rightHandClauseList)
     genOmpAtomicHintAndMemoryOrderClauses(converter, *rightHandClauseList, hint,
mlir::Type varType =
           converter.genExprValue(
               *Fortran::semantics::GetExpr(assignmentStmtVariable), stmtCtx))
           .getType();
   SmallVector<Type> varTys = {varType};
   SmallVector<Location> locs = {currentLocation};
   firOpBuilder.createBlock(&atomicUpdateOp.getRegion(), {}, varTys, locs);
   mlir::Value val =
       fir::getBase(atomicUpdateOp.getRegion().front().getArgument(0));
-  converter.bindSymbol(*updateSymbol, val);
+  auto varDesignator =
+      std::get_if<Fortran::common::Indirection<Fortran::parser::Designator>>(
+          &assignmentStmtVariable.u);
+  assert(varDesignator && "Variable designator for atomic update assignment "
+                          "statement does not exist");
+  const auto *name = getDesignatorNameIfDataRef(varDesignator->value());
+  assert(name && name->symbol &&
+         "No symbol attached to atomic update variable");
+  converter.bindSymbol(*name->symbol, val);
   // Set the insert for the terminator operation to go at the end of the
   // block.
   mlir::Block &block = atomicUpdateOp.getRegion().back();
   firOpBuilder.setInsertionPointToEnd(&block);
 
   mlir::Value result = fir::getBase(converter.genExprValue(
       *Fortran::semantics::GetExpr(assignmentStmtExpr), stmtCtx));
   // Insert the terminator: YieldOp.