This is an archive of the discontinued LLVM Phabricator instance.

[LLVM] Use after move bug fixes
Needs ReviewPublic

Authored by Prazek on Dec 14 2016, 4:19 AM.

Details

Summary

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

Event Timeline

Prazek updated this revision to Diff 81368.Dec 14 2016, 4:19 AM
Prazek retitled this revision from to Use after move bug fixes.
Prazek updated this object.
Prazek added reviewers: mehdi_amini, mboehme.
Prazek added a subscriber: llvm-commits.
malcolm.parsons added inline comments.
lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp
42–43

False positive - extract() reinitializes.

87

False positive - extract() reinitializes.

mboehme added inline comments.Dec 14 2016, 5:46 AM
lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp
42–43

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

mboehme added inline comments.Dec 14 2016, 6:09 AM
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 added inline comments.
lib/Object/Error.cpp
94

@lhames : can you look at this?

mehdi_amini added inline comments.Dec 14 2016, 8:34 AM
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 retitled this revision from Use after move bug fixes to [LLVM] Use after move bug fixes.Dec 14 2016, 8:36 AM
Prazek added inline comments.Dec 14 2016, 8:45 AM
lib/DebugInfo/DWARF/DWARFDebugAbbrev.cpp
42–43

Ok, I gonna leve comment here

Prazek added inline comments.Dec 17 2016, 4:44 AM
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 added a subscriber: vsk.Dec 17 2016, 4:07 PM
vsk added inline comments.
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 added inline comments.Dec 17 2016, 4:11 PM
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 added inline comments.Dec 17 2016, 4:13 PM
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 added inline comments.
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 updated this revision to Diff 82409.Dec 23 2016, 4:43 AM
Prazek marked 7 inline comments as done.

Addressing comments

Prazek added inline comments.Dec 23 2016, 4:46 AM
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 marked an inline comment as done.Dec 23 2016, 2:40 PM
Prazek added inline comments.
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 added inline comments.Jan 9 2017, 8:08 AM
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 added inline comments.Jan 9 2017, 10:56 AM
lib/Target/X86/X86ISelLowering.cpp
27197

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

Prazek added inline comments.Feb 9 2017, 11:53 AM
lib/Target/X86/X86ISelLowering.cpp
27197

have you checked this one Martin?

mboehme edited edge metadata.Feb 14 2017, 7:34 AM

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.

Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 1:08 AM
Herald added subscribers: hoy, wlei, pengfei. · View Herald Transcript
lhames added inline comments.Jul 19 2023, 3:09 PM
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 added inline comments.Jul 19 2023, 3:43 PM
lib/Object/Error.cpp
94

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