Replace confusing note in cpp.md on "RVO" #2327
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The original commit adding this note referenced rust-lang#778, but then went on to talk about how the problem was RVO and that the problem is not possible to solve in bindgen because bindgen can't know if RVO happened or not. However this is incorrect in several ways. The problem in rust-lang#778 has nothing whatsoever to do with RVO but rather with bindgen simply not understanding the calling convention for passing or returning types that are "non trivial for the purposes of calls". This is completely consistent and does not depend on what the optimizer decided to do (after all, if callsite side calling-convention did depend on what the optimizer happened to do inside the function it would be impossible for C++ compilers to emit correct calls to external C++ functions!). You can see this quite clearly [here](https://godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGIM6SuADJ4DJgAcj4ARpjEEgDspAAOqAqETgwe3r7%2ByanpAiFhkSwxcVyJdpgOGUIETMQEWT5%2BAVU1AnUNBEUR0bEJtvWNzTltwz2hfaUDFQCUtqhexMjsHAD06wDUACoAnkmYW3tLxFtoWFsIsZikWyRbtKhM6FuGW5iqrEn0AHQmGgAggoCMQvA4tgRMCCokwlFsTPErPEACImADMViBILBEKhIIRSIBgK2pMh0IIEDmGKxJLJ%2BMpaAYBIZ5gAbNTMcSyVsAG6oPDoABUkJpxMRaK5QIZWwUAEcvA1MFTCbSeTKCIjkZK1WTiJgCMsGKKpYCJeKgaECFsWExQiqtdz6RTIQj0SjZQqlVSxUCnaT9YbiMa0gAvTCoKgQBmc2kSjgLWicACsvD8HC0pFQnDc1msstOqwRZnRPFIBE0CYWAGsQOj0b9603my22fpOJI05Ws5xeAoQBpy5WFnBYEg0CwknRYuRKBOp/Q4sAFMwkgoEKgCKQsLy8KsAGp4TAAdwA8od02WaLQocR%2BxAot2oqEGntOGWJ2xBKeGLQ3xneCwW0jHEADtzwfUal5aFu0%2BaovChd9eCtTAkzA2g8CiYhXw8LBu1BPAWCQhYqAMZdDxPc9GCQmRBBEMR2CkWj5CUNRu10Lh9EMYw80sfRMP7SAFlQJJHAEfsOD7VDqjEvwIFcUY/E44IphKMo9BSNJZMUjT8lk3o1NmWxpI6BguhGTwWj0dpZPMyZin6cohm6HTOJBboDMciQFgUQtGMTFMuzA7MOC2VQAA42QAWjZSQtmAZBkC2aMwQYas5mS3BCAecxSzmXgKwAuYazrBsW3Kps2zQztSHTTMQr7AchyK0hR0QFBUEnacyAoCB526kBl1XddN23TBdwPI8zwvGjr1ve9HzA59mGIf8P06r8CB/P9uyA7jQMzfBIMcaCJMzODkAQtYyxQtDMwwrCcIwNZMwIojuATPgyIUCjpuoj7mPo8QmP4QRFBUdQwN0AIDCMFBeJsB7BKpLNRIyCSoqoBhUCi1DBUwKKmRxcEiDvKT7FklwGHcSycmU6nPJmJzNIKTJaaUvItIyRn1LckzbImVzjIp2oJh5oz3Is7IOcl%2Bzpl5ny/O89sOFTWruxCsLIpiuKEqSlKvDSjKICy0nizygrhxK%2BtGwq8qVZqureAa2wmsKrRipVswgvq3tmo9hZoLvDIQEkIA) Notice how the body of `main` is identical no matter if copy elision is enabled or not. To get `test` passed in a register you must remove the copy constructor. I spent a few hours being really confused by this note before I tracked down the original PR and realized what it was trying to talk about. Hopefully this saves the next person to come across it the same trouble. As an aside I suspect the clang c bindings can already give you information on special member functions and base classes, making it fairly easy to do this correctly. AFAICT bindgen already does not rely on llvm's code generation facilities to figure out the ABI of calls for C (except perhaps indirectly, via rustc), so this seems reasonable to just do in bindgen, explicitly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original commit adding this note referenced #778, but then went on to talk about how the problem was RVO and that the problem is not possible to solve in bindgen because bindgen can't know if RVO happened or not. However this is incorrect in several ways. The problem in #778 has nothing whatsoever to do with RVO but rather with bindgen simply not understanding the calling convention for passing or returning types that are "non trivial for the purposes of calls". This is completely consistent and does not depend on what the optimizer decided to do (after all, if callsite side calling-convention did depend on what the optimizer happened to do inside the function it would be impossible for C++ compilers to emit correct calls to external C++ functions!).
You can see this quite clearly here Notice how the body of
mainis identical no matter if copy elision is enabled or not. To gettestpassed in a register you must remove the copy constructor.I spent a few hours being really confused by this note before I tracked down the original PR and realized what it was trying to talk about. Hopefully this saves the next person to come across it the same trouble.
As an aside I suspect the clang c bindings can already give you information on special member functions and base classes, making it fairly easy to do this correctly. AFAICT bindgen already does not rely on llvm's code generation facilities to figure out the ABI of calls for C (except perhaps indirectly, via rustc), so this seems reasonable to just do in bindgen, explicitly.