This is an archive of the discontinued LLVM Phabricator instance.

[ORC] fix use-after-move. NFC
ClosedPublic

Authored by nickdesaulniers on May 19 2019, 6:09 PM.

Details

Summary

scan-build flagged a potential use-after-move in debug builds. It's not
safe that a moved from value contains anything but garbage. Manually
DRY up these repeated expressions.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2019, 6:09 PM
  • git-clang-format HEAD~
Harbormaster completed remote builds in B32127: Diff 200191.
nickdesaulniers retitled this revision from [ORC] fix use-after-move to [ORC] fix use-after-move. NFC.May 19 2019, 6:11 PM
lhames accepted this revision.May 20 2019, 2:50 PM

LGTM! Thanks Nick!

This revision is now accepted and ready to land.May 20 2019, 2:50 PM
This revision was automatically updated to reflect the committed changes.

Hey @lhames - is this code tested? I guess they're only debug output, so might not be tested. (was the use-after-move likely to be UB (null deref, etc) or just likely to log not what was intended?)

Reckon it'd be suitable to not use "auto" for the N and/or ES variables? The "N" variable perhaps makes me more concerned than ES - because is it a string reference? (that might be invalidated by the move), a return by value (that then means it's reference extending, which is best avoided unless really needed), etc?

because is it a string reference? (that might be invalidated by the move), a return by value (that then means it's reference extending, which is best avoided unless really needed),

Do you have links or more info to these problems? I've never heard the term "reference extending" before.

because is it a string reference? (that might be invalidated by the move), a return by value (that then means it's reference extending, which is best avoided unless really needed),

Do you have links or more info to these problems? I've never heard the term "reference extending" before.

Here's some info on reference lifetime extension: https://abseil.io/tips/107

As for string reference invalidation, I was picturing something like this:

struct foo { std::string s; const std::string &get_foo() { return s; } }; //struct isn't necessary, but to make it closer to the example
void func(foo f) {
  const std::string &ref = f.get_foo();
  foo g = std::move(f);
  /* do things with 'ref' here */
}

but at the last line there, the string inside 'f' has been moved into 'g', so the reference refers to the std::string in 'f' which is now in a valid but unspecified state (could contain the original string data (probably does if it's short enougH), or could point to a zero-length/empty string (if the data was too long and was dynamically allocated, and thus the allocation was moved over to the string in 'g' instead))