Skip to content

[BUG] _parameter-direction_ can't pass const address of fundamental type #696

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

Closed
JohelEGP opened this issue Sep 23, 2023 · 9 comments
Closed
Labels
bug Something isn't working

Comments

@JohelEGP
Copy link
Contributor

JohelEGP commented Sep 23, 2023

Title: parameter-direction can't pass const address of fundamental type.

Description:

Cpp2 in parameters don't guarantee aliasing their corresponding argument,
or something like that (e.g., the guarantee to not alias is implementation-defined).
This is my best guess, as there's no public documentation for Cpp2.
#686 argues that this should be less observable for the cases in parameters alias.

The other side of the coin is that
you can't pass a const object to a Cpp2 parameter and be guaranteed aliasing.
I'm going to beat the dead horse with the usual example
because I can't currently come up with an use case where I need this.
But I do have a suggestion that, while it borders on clever (or is, for you), might just work.

This issue means that you can't translate Cpp1 function signatures like the following one
to Cpp2 with just parameter-directions
without having to resort to type traits such as std::add_lvalue_reference_t<const int>.

const int& min(const int&, const int&);

That's a concrete version of std::min.
The following would work in this case (https://cpp2.godbolt.org/z/38WEGK8vr):

min: (
  l: std::add_lvalue_reference_t<const int>,
  r: std::add_lvalue_reference_t<const int>
) -> forward const int
= std::min(l, r);

So this occured to me:

const means "you can't write to it". So you cannot use inout with it. Simple as that.

... which made me realize that const qualification doesn't make sense for parameters (except copy parameters, because only those are expressing the intent of really making a new variable... all the others are conceptually, and usually actually, aliases for the original argument).

in parameters are optimized, and the difference is observable (#686).
How about allowing inout _: const _ parameters to mean an in parameter with an address?
That way, we can implement std::min (concrete or generic) in Cpp2.

-- #250 (comment).

This made my think about parameter aliasing.

One way to look at the suggestion of allowing inout _: const _ parameters
is that out and inout allow modifying passed arguments, and thus alias.
By adding const to inout, we remove the modification aspect, but retain aliasing.

Another thought of mine is making all in parameters not alias if that was possible.
And to require the inout _: const _ spelling for otherwise aliasing in parameters.
Perhaps that way we can get a better default and more performance,
like Rust parameters as I once understood them (I think it was after watching a talk by David Sankel).

-- #250 (comment).

Of course, there was some resistance (as there always is when suggesting complicating parameter directions).
Like using pointers, which I might, were I writing a new interface.
But this issue will definitely come up when translating Cpp1 to Cpp2.

Minimal reproducer (https://cpp2.godbolt.org/z/jW71x6sv6):

min: (
  l: int,
  r: int
) -> forward const int
= std::min(l, r);
main: () = {
  a := 0;
  b := 1;
  [[assert: min(a, b)& == a&]]
}
Commands:
cppfront main.cpp2
clang++17 -std=c++23 -stdlib=libc++ -lc++abi -pedantic-errors -Wall -Wextra -Wconversion -Werror=unused-result -I . main.cpp

Expected result:

Program returned: 0.

Also, as pointed out by #686, similar behavior whether the chosen parameter type aliases or not.
And considering #666, I'm against actually making in parameters always alias.

As pointed out in the description,
I'd expect this result if inout _: const _ was enabled
to mean something like "in parameter with guaranteed aliasing".

Actual result and error:

build/main.cpp:26:17: warning: reference to stack memory associated with parameter 'l' returned [-Wreturn-stack-address]
   26 | return std::min(l, r);  }
      |                 ^
build/main.cpp:26:20: warning: reference to stack memory associated with parameter 'r' returned [-Wreturn-stack-address]
   26 | return std::min(l, r);  }
      |                    ^
2 warnings generated.
Program returned: 139
Contract violation
libc++abi: terminating
Program terminated with signal: SIGSEGV
Cpp2 lowered to Cpp1:
//=== Cpp2 type declarations ====================================================


#include "cpp2util.h"



//=== Cpp2 type definitions and function declarations ===========================

[[nodiscard]] auto min(
  cpp2::in<int> l, 
  cpp2::in<int> r
) -> int const&;

auto main() -> int;
  

//=== Cpp2 function definitions =================================================

[[nodiscard]] auto min(
  cpp2::in<int> l, 
  cpp2::in<int> r
) -> int const& { 
return std::min(l, r);  }
auto main() -> int{
  auto a {0}; 
  auto b {1}; 
  cpp2::Default.expects(&min(a, std::move(b)) == &a, "");
}
@JohelEGP JohelEGP added the bug Something isn't working label Sep 23, 2023
@AbhinavK00
Copy link

I'm very curious about what Herb has to say about this.

I still think current behaviour is fine and the following should be enough to address this situation:

  1. if the return is by forward, then take in parameters by const&. Specifically the ones on whom the returned reference is dependent upon.
    Like in the example of min, since the return is by forward and returned reference is dependent on both the parameters, take them by const&.
    Ofcourse, this analysis could be hard and therefore it can be that every in parameter is a const& when there's a forward return.
  2. Forbid taking the address of in parameters. Why? Because one can always take their address and modify them, which goes against their in nature.

Now, I'm very sure that this will not suffice, but it works for the min example.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Sep 23, 2023

That's right.
The first point isn't enough as answered by #686 (comment):

That's just an easy way to manifest the difference in optimization.
You could depend on the in parameter's address some other way,
like pushing it back onto a vector.

The second point would help #686.

@AbhinavK00
Copy link

Sorry, but I don't understand the example of pushing onto a vector. Can you please explain.

@hsutter
Copy link
Owner

hsutter commented Sep 25, 2023

I'm trying to grok the question. Two key points:

  • in parameters are designed to be either by-values or by-pointer, whatever is most efficient.
  • By-pointer parameters might alias.

General note: The intent for aliasing and other lifetime issues is to pursue the direction of the C++ Core Guidelines Lifetime safety profile which is not yet implemented in cppfront.

Minimal reproducer (https://cpp2.godbolt.org/z/jW71x6sv6):

min: (
  l: int,
  r: int
) -> forward const int
= std::min(l, r);
main: () = {
  a := 0;
  b := 1;
  [[assert: min(a, b)& == a&]]
}

The intent was that this would be implemented using forward parameters:

min: (
  forward l: int,
  forward r: int
) -> forward const int
= std::min(l, r);

main: () = {
  a := 0;
  b := 1;
  [[assert: min(a, b)& == a&]]
}

This compiles and runs successfully, which seems right.

Does that help? I'm reading quickly here, so sorry if I missed something. Thanks for the feedback!

@JohelEGP
Copy link
Contributor Author

That works in this case (https://cpp2.godbolt.org/z/66e76d8K8).

I see some drawbacks in the general case:

  • We end up with a function template (e.g., &function-name works in less contexts).
  • The parameter isn't guaranteed to be const, even though that suffices.
  • We might have to explicitly use std::as_const on the parameter in the body.
  • Using a forward parameter for guaranteed aliasing is too big of a jump from an in parameter.
  • As Cppfront changes what's an optimized in parameter,
    or code is ported to Cpp2 compilers with different optimization heuristics,
    incidental in parameters that used to alias will have to be changed to use forward parameters
    (this was a bug in user's code, anyways, which is what [BUG] in parameter optimization should be less observable #686 is for).

So I think my suggested inout _: const _ would be better.

@JohelEGP
Copy link
Contributor Author

  • Using a forward parameter for guaranteed aliasing is too big of a jump from an in parameter.

And forward parameters are generally just for forwarding.
There's a bit of relaxation around that to allow things like logging first.
So they're really unfit for "aliasing in parameters".

@hsutter
Copy link
Owner

hsutter commented Sep 26, 2023

OK, those are reasonable concerns. For a while I wondered whether we'd have demand for an always-copy parameter, and it turns out we do, so I added copy... yes, there are lots of useful use cases, including concurrency, when you want to know there will be no aliasing.

So the question is "are there use cases when you know you want to guarantee aliasing?" Normally for a feature extension I'd say "I want to be driven by use cases, and min is a good one, what others are there?" (Regardless of whether std::min having reference semantics is a good idea, it's a concrete example of code we might want to be able to express. I could imagine a similar "return a read-only reference to whichever is the least-loaded server" or something like that, although that would work with in out of the box because servers aren't cheap to copy.)

But in this case I think there's a short cut, because this isn't actually a feature extension, or at least not as much: The in x: const T idiom already naturally works, except only for a special check that disallows it, so I just need to comment out that one check and it would work. So let's try that and we can gather usage experience to see when it's actually used.

I think this will be a worthwhile experiment! Thanks for persisting on this, I appreciate it.

hsutter added a commit that referenced this issue Sep 26, 2023
@hsutter
Copy link
Owner

hsutter commented Sep 26, 2023

(Note the first commit mentioned immediately above says it closed this issue, but that was a stray commit comment. The second one immediately following it is the one that closed this.)

Thanks!

@JohelEGP
Copy link
Contributor Author

From #666, another use case:

#270 (comment) describes the general problem.
However, it generally doesn't apply to Cpp2.
Cpp2 generally requires parameters of complete type.
That's because most functions are initialized and will have a Cpp1 definition emitted.
So we can generally try to optimize in parameters of class types to pass-by-value.
I'd love to have this optimization restored for the general case.

This might not be true anymore given that
.h2 headers permit splitting the interface and implementation to different TUs
(see #705 (comment)).

Working around such an issue might be another use case for inout _: const _ parameters (#696).
Opting out of the in optimization is certainly a use case for inout _: const _.
Certain things in https://wg21.link/temp are specified with lexical analysis (AFAIU).
If the same kind of analysis to not optimize in parameters falls short, reach out for inout _: const _.

zaucy pushed a commit to zaucy/cppfront that referenced this issue Dec 5, 2023
zaucy pushed a commit to zaucy/cppfront that referenced this issue Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants