stan-dev / stanc3

The Stan transpiler (from Stan to C++ and beyond).
BSD 3-Clause "New" or "Revised" License
138 stars 44 forks source link

Avoid passing temporaries as const ref to std::tuple #1382

Closed WardBrian closed 6 months ago

WardBrian commented 6 months ago

First reported by @goedman here, there seems to be a clang-specific issue with passing temporaries like those produced by stan::math::multiply to the constructor of std::tuple when std::tuple is told it will be holding a constant reference.

GCC seems fine with this, and @SteveBronder thinks the C++ spec allows it, but we work around it here by restricting the logic we used to mark a type in a tuple as constant reference to only simple expressions (e.g., variables)

Submission Checklist

Release notes

Fixed an issue with clang compilers and constructing tuples.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

WardBrian commented 6 months ago

Here is a really minimal godbolt showing the issue in clang but not GCC:

https://godbolt.org/z/E48P35vbx

It seems like the issue began in Clang 7.0.0, earlier versions compiled it fine.

codecov[bot] commented 6 months ago

Codecov Report

Merging #1382 (ec1d544) into master (02a3a85) will increase coverage by 0.00%. Report is 3 commits behind head on master. The diff coverage is 100.00%.

:exclamation: Current head ec1d544 differs from pull request most recent head 9622dea. Consider uploading reports for the commit 9622dea to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1382 +/- ## ======================================= Coverage 89.93% 89.93% ======================================= Files 63 63 Lines 10699 10701 +2 ======================================= + Hits 9622 9624 +2 Misses 1077 1077 ``` | [Files](https://app.codecov.io/gh/stan-dev/stanc3/pull/1382?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stan-dev) | Coverage Δ | | |---|---|---| | [src/stan\_math\_backend/Lower\_expr.ml](https://app.codecov.io/gh/stan-dev/stanc3/pull/1382?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stan-dev#diff-c3JjL3N0YW5fbWF0aF9iYWNrZW5kL0xvd2VyX2V4cHIubWw=) | `92.85% <100.00%> (+0.03%)` | :arrow_up: |
WardBrian commented 6 months ago

I found a similar issue for another library which may imply that this is something Eigen could fix: https://github.com/nlohmann/json/issues/2226

Still not clear why gcc is happier than clang about it

WardBrian commented 6 months ago

@SteveBronder and I spent a lot more time looking over it and we actually think clang may be correct (executing the gcc version in godbolt segfaults, but this version does not)

We also found some issues related to data and eigen::maps which were similar