[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

False positive - extract() reinitializes.

85

False positive - extract() reinitializes.

[mboehme]

lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp

42

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"

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

27000

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

53

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

Ok, I gonna leve comment here

[Prazek]

lib/Target/X86/X86ISelLowering.cpp

27000

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

53

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

53

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

53

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

53

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

53

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

[Prazek]

tools/llvm-profdata/llvm-profdata.cpp

53

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

27000

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

27000

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

[Prazek]

lib/Target/X86/X86ISelLowering.cpp

27000

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

27000

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));
+    Decls.push_back(AbbrDecl);
   }
   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;
-    AbbrDeclSets[CUAbbrOffset] = std::move(AbbrDecls);
+    AbbrDeclSets[CUAbbrOffset] = 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 Indentifier = 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,
+        Err = SMDiagnostic(std::move(Indentifier), 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));
   } else if (usesLayout<DoubleAPFloat>(Semantics)) {
-    new (&Double)
-        DoubleAPFloat(Semantics, APFloat(std::move(F), F.getSemantics()),
+    new (&Double) DoubleAPFloat(Semantics, APFloat(F, F.getSemantics()),
                                 APFloat(IEEEdouble));
   } else {
     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)) {
+    // No use after move, WidenMask reinitialized in canWidenShuffleElements.
     Mask = std::move(WidenedMask);
   }
 
   // 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

static void exitWithError(const Twine &Message, StringRef 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);
     });
+    LLVM_BUILTIN_UNREACHABLE;
   }
-
   exitWithError(toString(std::move(E)), Whence);
 }
 
 static void exitWithErrorCode(std::error_code EC, StringRef Whence = "") {
   exitWithError(EC.message(), Whence);
 }
 
 namespace {