New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 409318 link

Starred by 18 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

Clang should detect argument order of evaluation issues with smart pointers and std::move

Project Member Reported by r...@chromium.org, Aug 30 2014

Issue description

This code has a bug:

  task_runner_->PostTaskAndReply(
      FROM_HERE,
      base::Bind(&WiFiService::GetProperties,
                 base::Unretained(wifi_service_.get()),
                 guid,
                 properties.get(),
                 error),
      base::Bind(&NetworkingPrivateServiceClient::AfterGetProperties,
                 weak_factory_.GetWeakPtr(),
                 service_callbacks->id,
                 guid,
                 base::Passed(&properties),
                 base::Owned(error)));

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/api/networking_private/networking_private_service_client.cc&l=177

The 'properties' variable is a scoped_ptr that is accessed and modified in the same function call. Unfortunately, order of evaluation of arguments isn't defined. In practice, everyone does right-to-left evaluation, and only Clang on non-Windows OSs does left-to-right. This code only works with Clang on non-Windows.

The matcher should basically just look for a CallExpr containing some scoped_ptr.get and base::Passed() on the same variable, it doesn't matter if they are in nested callexprs, as long as they are in the same statement the order is undefined.
 

Comment 2 by thakis@chromium.org, Aug 30 2014

I've been thinking if we could maybe teach clang's -Wunsequenced about this somehow, instead of special-casing ScopedPtr.

One idea is that SequenceChecker (in lib/Sema/SemaChecking.cpp, which implements this warning, look for warn_unsequenced_mod_use) could detect moves. This should then help with std::unique_ptr. It wouldn't help Chromium immediately, but we probably want to replace the move emulation we currently have with real moves once we can use c++11, and at that point this change should also find the issue described in this bug.

Comment 3 by thakis@chromium.org, Aug 30 2014

i.e. -Wunsequenced should warn on this:

#include <memory>

void f(int *p, std::unique_ptr<int> foo) {}

int main() {
  std::unique_ptr<int> p;
  f(p.get(), std::move(p));
}

Comment 4 by thakis@chromium.org, Aug 30 2014

…thinking more about this, since we won't get rid of base::Passed() immediately (we'd implement it like std::move<>() is implemented, is my guess), and since -Wunsequenced doesn't do any interprocedural analysis, this probably wouldn't help chromium.

Since std::unique_ptr is how one says "this takes ownership" in c++, it's probably the right approach in principle (filed http://llvm.org/PR20819 for that), but to get the benefit of a warning anytime soon in chromium, we probably have to basically reimplement the warning in the plugin after all. Oh well.
dcheng: thakis, awong: would it make sense to catch things like f(s.release(), s->foo()) too?
I did a build of all targets with that warning (on linux), it didn't find anything. I checked that it would've found the thing fixed by the cl in #1 (I changed my build locally to make that file build on linux).

So not sure if this is worth cleaning up and landing. (It subjectively feels like it makes builds slower, so maybe it would've to be off by default too. I didn't measure if builds are actually slower though.)

Comment 8 by dcheng@chromium.org, Sep 27 2014

Cc: scherkus@chromium.org akalin@chromium.org darin@chromium.org willchan@chromium.org
 Issue 161165  has been merged into this issue.
Cc: mtomasz@chromium.org
Labels: -Pri-2 -OS-Windows Pri-1 OS-All
Status: Available
We got a Pri-0 infinite crash loop bug (crbug.com/462999) because of this problem. It should be caught by tests, but it seems that we build them on buildbots and trybots with clang which does left-to-right evaluation. Release builds pushed to users are built with G++ though, which does right-to-left evaluation.

As these bugs are not easy to spot, I guess we have plenty of places in Chromium with them. Those bugs are especially scary for Chrome OS.
Cc: -scherkus@chromium.org
Cc: -willchan@chromium.org
Cc: manisca...@chromium.org
FWIW, I found another argument evaluation order bug that could have been caught by this sort of plugin.

https://codereview.chromium.org/1216403004/

thakis, do you think the approach you were looking at in #5 is still the way to go (until clang detects it natively)? If the increase in build time is small, it may be worth it.

If people think this is useful, I'd recommend measuring how much the patch in #5 slows down builds as a first step.
I'll also toss in my vote since such a checker would've prevented issue 516922.

It would have also prevented  issue 547056 
Also would've prevented issue 614900.

Now that we no longer have scoped_ptr and base::Passed(), we should probably try to implement this upstream (https://llvm.org/bugs/show_bug.cgi?id=27562), but if that proves difficult I suppose we could tweak the patch from comment 5 to work with unique_ptr / move() instead.
Labels: -Clang clang
Also would've prevented  issue 714609 .
(But there it was base::Passed(&uniqptr), so there was no std::move() involved and an upstream warning wouldn't have found this.)
In theory, once base::OnceCallback and base::RepeatingCallback are cleanly separated, we will remove base::Passed() and just use std::move().
I just saw another instance similar to this in Issue 727677 (https://chromium-review.googlesource.com/518423):
db->UpdateEntries(..., base::BindOnce(&OnDBUpdateComplete, std::move(db), ...

Also  issue 733319  as well now. =(

Comment 22 by r...@chromium.org, Aug 17 2017

Another instance:  issue 746971 

Comment 23 by r...@chromium.org, Aug 17 2017

Summary: Clang should detect argument order of evaluation issues with smart pointers and std::move (was: The Clang plugin should detect argument order of evaluation issues with scoped_ptr and base::Passed)
(retitling, since we don't have this custom pre-C++11 machinery anymore)
Components: Build
Owner: reillyg@chromium.org
Status: Assigned (was: Available)
I've never written a Clang plugin before but I'm interested in getting this done.
Implementing this as a plugin would be trivial, especially now that we use RecursiveASTVisitor. However, I believe thakis@ would like this implemented upstream in clang itself.

Sign in to add a comment