Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pydrake doc: gcc and clang disagree on need for lambda capture for constexpr vars #9600

Closed
2 tasks done
EricCousineau-TRI opened this issue Oct 5, 2018 · 11 comments · Fixed by #9606
Closed
2 tasks done

Comments

@EricCousineau-TRI
Copy link
Contributor

EricCousineau-TRI commented Oct 5, 2018

From here: #9571 (comment)

Workarounds:

  • Use constexpr auto& doc, rather than auto& doc
  • Use [&] in lambda capture
  • Add copts = ["-Wno-unused-lambda-capture"] for pybind_py_library

Action items:

\cc @m-chaturvedi @jamiesnape

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Oct 5, 2018

Possibly related; however, I am getting this with clang-6.0, and it'd seem that this should have been incorporated already:
https://bugs.llvm.org/show_bug.cgi?id=31815

EDIT: Nevermind; it seems exactly related. We have a generic lambda here. Will check versioning more closely.

EDIT 2: That was circumstantial; had to deal with storage duration inference via upstream auto& (determined before lambda definition).

@m-chaturvedi
Copy link
Contributor

Maybe related: https://reviews.llvm.org/D33526

@EricCousineau-TRI
Copy link
Contributor Author

Seems that this patch was incorporated before clang-6.0:

$ clang-6.0 --version
clang version 6.0.0-1ubuntu2~16.04.1 (tags/RELEASE_600/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

The branch in the Git repo release_60:
https://github.com/llvm-mirror/clang/tree/release_60
Does incorporate the corresponding git commit for the patch:
llvm-mirror/clang@6c70606

So perhaps it's still an issue?

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Oct 5, 2018

Huh, seems like you can also change auto& doc = ... to constexpr auto& doc = ..., and then neither clang nor gcc seem to care, due to how constexpr affects storage duration?

@jamiesnape
Copy link
Contributor

Very Java-like (except that would be final). Makes a certain amount of sense.

@EricCousineau-TRI
Copy link
Contributor Author

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Oct 6, 2018

Filed bug with clang, will see if they know where it should go:
https://bugs.llvm.org/show_bug.cgi?id=39202

@EricCousineau-TRI
Copy link
Contributor Author

From chat with @m-chaturvedi, seems that GCC 7.3.0 on Bionic decided to further deviate from clang; with the merge of #9606, we now get a Bionic failure:
https://drake-jenkins.csail.mit.edu/job/linux-bionic-gcc-bazel-experimental/11/console

bindings/pydrake/systems/framework_py_semantics.cc:72:37: note: the lambda has no capture-default
   auto bind_common_scalar_types = [m](auto dummy) {
                                     ^
bindings/pydrake/systems/framework_py_semantics.cc:31:19: note: 'constexpr const<unnamed struct>::<unnamed struct>::<unnamed struct>& doc' declared here
   constexpr auto& doc = pydrake_doc.drake.systems;
                   ^~~
bindings/pydrake/systems/framework_py_semantics.cc:310:12: error: 'doc' is not captured
            doc.Parameters.SetFrom.doc);

Will submit a chaser PR to use the second workaround :(

@EricCousineau-TRI
Copy link
Contributor Author

I may have spoken too soon? I am unable to reproduce this with simple reproduction:
EricCousineau-TRI/repro@4109dc4?diff=split#diff-bc917a88a9cbeb890e61e5fb340a8ec5

Even trying to re-simulate the nested structs does not yield an issue:
EricCousineau-TRI/repro@bbc8f78?diff=split#diff-bc917a88a9cbeb890e61e5fb340a8ec5R18

Not sure what the discrepancy is... @m-chaturvedi @jamiesnape Might I ask if y'all have an inkling?

@EricCousineau-TRI
Copy link
Contributor Author

EricCousineau-TRI commented Oct 8, 2018

Mmanu found that this error can be reproduced if we use a generic / templated lambda, such as using auto as a parameter:
EricCousineau-TRI/repro@fd025bc?diff=split#diff-bc917a88a9cbeb890e61e5fb340a8ec5

Seems related to old aforementioned bug with LLVM, but now specific to GCC: https://bugs.llvm.org/show_bug.cgi?id=31815

@EricCousineau-TRI
Copy link
Contributor Author

Posted a GCC bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87559

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants