[LLVM] Use after move bug fixes

[Prazek]

Summary

Bunch of fixed bugs in LLVM afte running misc-use-after-move analysis

[malcolm.parsons]

lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

42–43

False positive - extract() reinitializes.

87

False positive - extract() reinitializes.

[mboehme]

lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

42–43

Thanks. FYI, we're working on a proposal for annotating methods that reinitialize an object.

[mboehme]

lib/IRReader/IRReader.cpp

39

Typo: Should be "Identifier"

43–44

This is a StringRef -- no need to move it (don't think it has a move constructor anyway as a move would not be cheaper than a copy).

lib/Target/X86/X86ISelLowering.cpp

27197

Typo: Should be WidenedMask

tools/llvm-profdata/llvm-profdata.cpp

63

Inadvertently deleted empty line?

[mehdi_amini]

lib/Object/Error.cpp

94

@lhames : can you look at this?

[mehdi_amini]

tools/llvm-profdata/llvm-profdata.cpp

52

I'm wondering why there is a check for error before calling handleAllErrors here? (@lhames while you're at it...)

[Prazek]

lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

42–43

Ok, I gonna leve comment here

[Prazek]

lib/Target/X86/X86ISelLowering.cpp

27197

btw, why it fires here? It is passed to canWidenShuffleElements by reference and documentation says that it assumes it is being reinitialized in this case.

[vsk]

tools/llvm-profdata/llvm-profdata.cpp

52

If it is an instrprof error, we'd like the chance to inspect it and display an additional hint, instead of simply printing out a string representation of the error.

[mehdi_amini]

tools/llvm-profdata/llvm-profdata.cpp

52

The check still does not seem idiomatic to me, it could be instead:

    E = handleErrors(std::move(E), [&](const InstrProfError &IPE) {
     instrprof_error instrError = IPE.get();
      StringRef Hint = "";
      if (instrError == instrprof_error::unrecognized_format) {
        // Hint for common error of forgetting -sample for sample profiles.
        Hint = "Perhaps you forgot to use the -sample option?";
      }
      exitWithError(IPE.message(), Whence, Hint);
    });
    exitWithError(toString(std::move(E)), Whence);
`

[mehdi_amini]

tools/llvm-profdata/llvm-profdata.cpp

52

I *think* you can also write:

    handleAllErrors(std::move(E),
      [&](const InstrProfError &IPE) {
       instrprof_error instrError = IPE.get();
        StringRef Hint = "";
        if (instrError == instrprof_error::unrecognized_format) {
          // Hint for common error of forgetting -sample for sample profiles.
          Hint = "Perhaps you forgot to use the -sample option?";
        }
        exitWithError(IPE.message(), Whence, Hint);
      },
      [&](const Error &E) {
        exitWithError(toString(E), Whence);
    });
`

[Prazek]

lib/Object/Error.cpp

94

ping

tools/llvm-profdata/llvm-profdata.cpp

52

Both seems fine, but I guess only the first one will get rid of the warning, because compiler will not be able to reason that handleErrors is noreturn because each lambda is noreturn

[Prazek]

Addressing comments

[Prazek]

tools/llvm-profdata/llvm-profdata.cpp

52

oh ok I got it, we get rid of the second exitWithError call and use after move

[Prazek]

tools/llvm-profdata/llvm-profdata.cpp

52

ok, the code right now doesn't compile here, but I will fix it (toString takes E by value)

[mboehme]

lib/Target/X86/X86ISelLowering.cpp

27197

You're right, it shouldn't fire here. I'll take a look at what's going wrong here.

[Prazek]

lib/Target/X86/X86ISelLowering.cpp

27197

I was also surprised. Thanks Martin for taking care about it

[Prazek]

lib/Target/X86/X86ISelLowering.cpp

27197

have you checked this one Martin?

[mboehme]

What's the status of this change in general? There seem to be a couple of comments that are still open?

lib/Target/X86/X86ISelLowering.cpp

27197

Sorry, no -- I've been busy with other things.

[lhames]

lib/Object/Error.cpp

94

We can just return Error::success() instead of Err.

The reasoning is:

1. if Err is a success value then handleErrors will return Error::success() and we'll fall through to the implicit else case (currently return Err;). Returning Error::success() from the implicit else case maintains the expected behavior.
2. if Err is an ECError failure value with error code object_error::invalid_file_type then we will return Error::success from handleErrors, and again fall through to the implicit else case. Returning Error::success() from the implicit else maintains the expected behavior.
3. if Err is any other kind of failure value then we will return it from handleErrors, and then from the return in the then case, and the implicit else case does not affect the outcome.

[mehdi_amini]

lib/Object/Error.cpp

94

7 years latency on the ping, @lhames you beat a record here ;)

lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

if (FirstAbbrCode == 0) {
       FirstAbbrCode = AbbrDecl.getCode();
     } else {
       if (PrevAbbrCode + 1 != AbbrDecl.getCode()) {
         // Codes are not consecutive, can't do O(1) lookups.
         FirstAbbrCode = UINT32_MAX;
       }
     }
     PrevAbbrCode = AbbrDecl.getCode();
-    Decls.push_back(std::move(AbbrDecl));
+    // Extract reinitializes AbbrDecl, so there is no use after move here.
+    Decls.push_back(std::move(AbbrDecl)); // NOLINT(misc-use-after-move)
   }
   return BeginOffset != *OffsetPtr;
 }
 
 void DWARFAbbreviationDeclarationSet::dump(raw_ostream &OS) const {
   for (const auto &Decl : Decls)
     Decl.dump(OS);
 }
void DWARFDebugAbbrev::extract(DataExtractor Data) {
   clear();
 
   uint32_t Offset = 0;
   DWARFAbbreviationDeclarationSet AbbrDecls;
   while (Data.isValidOffset(Offset)) {
     uint32_t CUAbbrOffset = Offset;
     if (!AbbrDecls.extract(Data, &Offset))
       break;
+    // NOLINT(misc-use-after-move) Extract reinitializes AbbrDecls.
     AbbrDeclSets[CUAbbrOffset] = std::move(AbbrDecls);
   }
 }
 
 void DWARFDebugAbbrev::dump(raw_ostream &OS) const {
   if (AbbrDeclSets.empty()) {
     OS << "< EMPTY >\n";
     return;
   }

lib/IRReader/IRReader.cpp

 static const char *const TimeIRParsingName = "parse";
 static const char *const TimeIRParsingDescription = "Parse IR";
 
 static std::unique_ptr<Module>
 getLazyIRModule(std::unique_ptr<MemoryBuffer> Buffer, SMDiagnostic &Err,
                 LLVMContext &Context, bool ShouldLazyLoadMetadata) {
   if (isBitcode((const unsigned char *)Buffer->getBufferStart(),
                 (const unsigned char *)Buffer->getBufferEnd())) {
+    auto Identifier = Buffer->getBufferIdentifier();
     Expected<std::unique_ptr<Module>> ModuleOrErr = getOwningLazyBitcodeModule(
         std::move(Buffer), Context, ShouldLazyLoadMetadata);
     if (Error E = ModuleOrErr.takeError()) {
       handleAllErrors(std::move(E), [&](ErrorInfoBase &EIB) {
-        Err = SMDiagnostic(Buffer->getBufferIdentifier(), SourceMgr::DK_Error,
-                           EIB.message());
+        Err = SMDiagnostic(Identifier, SourceMgr::DK_Error, EIB.message());
       });
       return nullptr;
     }
     return std::move(ModuleOrErr.get());
   }
 
   return parseAssembly(Buffer->getMemBufferRef(), Err, Context);
 }

lib/Object/Error.cpp

if (auto Err2 =
               return Error::success();
 
             // We failed to handle 'M' - return it from the handler.
             // This value will be passed back from catchErrors and
             // wind up in Err2, where it will be returned from this function.
             return Error(std::move(M));
           }))
     return Err2;
-  return Err;
+  return Err; // Use after move, not sure how to fix it.
 }

lib/Support/APFloat.cpp

 } // End detail namespace
 
 APFloat::Storage::Storage(IEEEFloat F, const fltSemantics &Semantics) {
   if (usesLayout<IEEEFloat>(Semantics)) {
     new (&IEEE) IEEEFloat(std::move(F));
     return;
   }
   if (usesLayout<DoubleAPFloat>(Semantics)) {
-    new (&Double)
-        DoubleAPFloat(Semantics, APFloat(std::move(F), F.getSemantics()),
+    new (&Double) DoubleAPFloat(Semantics, APFloat(F, F.getSemantics()),
                                 APFloat(semIEEEdouble));
     return;
   }
   llvm_unreachable("Unexpected semantics");
 }
 
 APFloat::opStatus APFloat::convertFromString(StringRef Str, roundingMode RM) {
   return getIEEE().convertFromString(Str, RM);
 }

lib/Target/X86/X86ISelLowering.cpp

static bool combineX86ShufflesRecursively(ArrayRef<SDValue> SrcOps,
 
   // Minor canonicalization of the accumulated shuffle mask to make it easier
   // to match below. All this does is detect masks with sequential pairs of
   // elements, and shrink them to the half-width mask. It does this in a loop
   // so it will reduce the size of the mask to the minimal width mask which
   // performs an equivalent shuffle.
   SmallVector<int, 16> WidenedMask;
   while (Mask.size() > 1 && canWidenShuffleElements(Mask, WidenedMask)) {
-    Mask = std::move(WidenedMask);
+    // No use after move, WidenedMask reinitialized in canWidenShuffleElements.
+    Mask = std::move(WidenedMask); // NOLINT(misc-use-after-move)
   }
 
   // Canonicalization of binary shuffle masks to improve pattern matching by
   // commuting the inputs.
   if (Ops.size() == 2 && canonicalizeShuffleMaskWithCommute(Mask)) {
     ShuffleVectorSDNode::commuteMask(Mask);
     std::swap(Ops[0], Ops[1]);
   }

lib/Transforms/Scalar/LoadCombine.cpp

for (auto &I : BB) {
     if (!LI)
       continue;
     ++NumLoadsAnalyzed;
     if (!LI->isSimple() || !LI->getType()->isIntegerTy())
       continue;
     auto POP = getPointerOffsetPair(*LI);
     if (!POP.Pointer)
       continue;
-    LoadMap[POP.Pointer].push_back({LI, std::move(POP), Index++});
+    LoadMap[POP.Pointer].push_back({LI, POP, Index++});
     AST.add(LI);
   }
   if (combineLoads(LoadMap))
     Combined = true;
   return Combined;
 }
 
 char LoadCombine::ID = 0;
 
 BasicBlockPass *llvm::createLoadCombinePass() {
   return new LoadCombine();
 }
 
 INITIALIZE_PASS_BEGIN(LoadCombine, "load-combine", LDCOMBINE_NAME, false, false)
 INITIALIZE_PASS_DEPENDENCY(AAResultsWrapperPass)
 INITIALIZE_PASS_END(LoadCombine, "load-combine", LDCOMBINE_NAME, false, false)

tools/llvm-profdata/llvm-profdata.cpp

if (!Whence.empty())
     errs() << Whence << ": ";
   errs() << Message << "\n";
   if (!Hint.empty())
     errs() << Hint << "\n";
   ::exit(1);
 }
 
 static void exitWithError(Error E, StringRef Whence = "") {
-  if (E.isA<InstrProfError>()) {
     handleAllErrors(std::move(E), [&](const InstrProfError &IPE) {
       instrprof_error instrError = IPE.get();
       StringRef Hint = "";
       if (instrError == instrprof_error::unrecognized_format) {
         // Hint for common error of forgetting -sample for sample profiles.
         Hint = "Perhaps you forgot to use the -sample option?";
       }
       exitWithError(IPE.message(), Whence, Hint);
+    },
+                    [&](const Error &E) {
+                      exitWithError(toString(E), Whence);
                     });
   }
-
-  exitWithError(toString(std::move(E)), Whence);
 }
 
 static void exitWithErrorCode(std::error_code EC, StringRef Whence = "") {
   exitWithError(EC.message(), Whence);
 }
 
 namespace {
 enum ProfileKinds { instr, sample };