Expression sequencing with chromium's version of clang and --std=cpp14

[alexc...@chromium.org]

Does the version clang we're using in chromium apply the C++17 sequencing rules for expressions with chained methods with --std=cpp14?

Consider the following:

std::unique_pointer<Foo> f;

A(f.get()).B(f.get()).C(std::move(f));

From what I understand prior to C++17 compilers where allowed to evaluate the expressions for chained functions in any order, potentially leading to a use after move bug in the above code (apparently we've seen that sort of bug with PostTaskAndReply, although that's not quite the same thing since there's only one function call). 

Thanks,

Alex Clarke

[Hans Wennborg]

I believe that Clang implements the C++17 evaluation order rules
regardless of -std= level, but I'm not 100% sure. Richard, can you
confirm?

[Richard Smith]

On Wed, Sep 5, 2018 at 1:32 AM Hans Wennborg <ha...@chromium.org> wrote:

I believe that Clang implements the C++17 evaluation order rules
regardless of -std= level, but I'm not 100% sure. Richard, can you
confirm?

Yes, that's correct; we use the same evaluation order (intending to match the C++17 rules) in all language modes.

However, Clang's -Wunsequenced warning will use the appropriate rules for the language mode, so you may still get "unsequenced" warnings even for code for which we provide a sequencing guarantee in practice. (As it happens, neither GCC nor Clang have updated their warning to follow the C++17 rules in C++17 mode yet -- https://godbolt.org/z/r15DG6 -- but you should expect that when they do, the update will only affect C++17 mode and not earlier language modes.)

[Alex Clarke]

Thanks for the info, that's very helpful!

[Alex Clarke]

A follow up question, is this also true for the nacl tool chain?

[Nico Weber]

Very likely no. The nacl toolchain is very old and doesn't know anything about C++17 yet. thomasanderson has been looking at updating it on and off, I don't know what the current status is.

--
You received this message because you are subscribed to the Google Groups "Clang maintainers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to clang+un...@chromium.org.

[Tom Anderson]

On Wed, Sep 5, 2018 at 12:13 PM Nico Weber <tha...@chromium.org> wrote:

Very likely no. The nacl toolchain is very old and doesn't know anything about C++17 yet. thomasanderson has been looking at updating it on and off, I don't know what the current status is.

Haven't worked on it recently, and unfortunately I wouldn't hold my breath on it :(

[Alex Clarke]

Ah well, we'll just have to adapt :)

[Gabriel Charette]

Should we try to enable -Wunsequenced in Chromium? That'd prevent the patterns we're most worried about prior to C++17 adoption.

You received this message because you are subscribed to the Google Groups "scheduler-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scheduler-de...@chromium.org.
To post to this group, send email to schedu...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CAPG_qM5eshCGvzB2dy1ec5dxTgJijawrSd0vYu0bonqWrXUT-A%40mail.gmail.com.

[Nico Weber]

Pretty sure it's already on? Are you not seeing it locally?

[Gabriel Charette]

I can't find it in the Chromium codebase, is it an on-by-default warning?

[Nico Weber]

Yes.

[Gabriel Charette]

I guess it doesn't do what I expect then because

    ThreadTaskRunnerHandle::Get()->PostTaskAndReply(FROM_HERE, std::move(closure), std::move(closure));

compiles.

Oh I see, it catches this:

    int MyMethod(int x, int y) {

      return x+y;

    }

    int x = 1;

    MyMethod(x++, x++);

error: multiple unsequenced modifications to 'x' [-Werror,-Wunsequenced]

So it is enabled, but it doesn't consider std::move a "modification" :(.

[Nico Weber]

Yup, that's https://bugs.chromium.org/p/chromium/issues/detail?id=409318 / http://llvm.org/PR20819

[Gabriel Charette]

Cool! Any ETA on this? I see the latest update was in January

[Nico Weber]

Nobody's actively working on it. Patches accepted :-)

[Etienne Pierre-doray]

There's also a clang tidy check for use after move:

https://clang.llvm.org/extra/clang-tidy/checks/bugprone-use-after-move.html

I'll test locally to see what it can catch. Not sure how much work it would be to have it running on trybots.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CAMGbLiH1Q-j9Jj_fKz2KdKFOmfV8bFat0Et4A5v7PWVMMm1x3g%40mail.gmail.com.

[Nico Weber]

The general thinking here is that high-value low-false-positive things like this should be compiler warnings part of the regular build. We do want to use clang-tidy for more advisory things asynchronously as a tricium tool that leaves comments on gerrit.