Skip to content

Add generics to safe, single-void functions #639

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
wants to merge 83 commits into from
Closed

Conversation

kyleheadley
Copy link
Member

@kyleheadley kyleheadley commented Jul 6, 2021

Marked draft pending merge. Again considered draft until cleanup.

This PR adds the ability for 3c to rewrite select function prototypes with generics. A variable with type void * is replaced by _For_any(T) ... _Ptr<T> ... when the variable is not wild from other sources. Function pointers are not converted.

Casting from void * is now a source of wildness unless the cast is of a generic function. The cast in this case is unnecessary, but retained for compatibility.

It would be helpful to have some comments about use of Internal vs External function constraints.
Additional testing ideas would be appreciated.

@mattmccutchen-cci
Copy link
Member

Just making a note here about the _Array_ptr<void> bug in this PR that we discussed yesterday to try to ensure we don't forget about it. As of 2ecd9ff, the following code (reduced from the actual declarations of recv in the glibc system headers and the Checked C system headers):

extern int recv0_alias(void *buf, int n);

int recv0(void *buf : itype(_Array_ptr<void>) byte_count(n), int n) {
  return recv0_alias(buf, n);
}

rewrites to:

extern int recv0_alias(void *buf, int n);

_Itype_for_any(T) int recv0(void *buf : itype(_Array_ptr<T>) byte_count(n), int n) {
  return recv0_alias(buf, n);
}

This is not safe because we don't know anything about what recv0_alias does to buf. The correct result is probably to not change the code at all. Some further details.

@kyleheadley
Copy link
Member Author

I've merged in the no_extra_atoms working branch now that that PR was merged, so this PR is mixed and should be considered as a draft until it can be cleaned up and re-posted. Current work is to get the benchmarks passing. The first of these only required a canWrite(), so I'm hoping the rest will require minimal code as well.

@kyleheadley kyleheadley changed the title Add generics to safe, single-void functions (Draft) Add generics to safe, single-void functions Aug 9, 2021
@mattmccutchen-cci
Copy link
Member

I've merged in the no_extra_atoms working branch now that that PR was merged

Note that merging one PR branch into another creates a risk of incorrect merging later, as described in the engineering handbook. We could help ensure a correct result by applying the procedure from the handbook for "If A is squash-merged to “main”" just before you next merge main into this PR (whenever you are ready to do that). Since it appears that what you merged into add_generic was not the final state of no_extra_atoms (2e439df) but some intermediate commit, we would need to merge 2e439df (which would pull in the LLVM 12 upgrade, among other things) before the procedure could be applied. I'll be happy to do all of this when you are ready to merge main into this PR. Or we can skip it and you can just be really careful when you merge from main.

@kyleheadley kyleheadley marked this pull request as draft August 9, 2021 17:47
@kyleheadley kyleheadley changed the title (Draft) Add generics to safe, single-void functions Add generics to safe, single-void functions Aug 9, 2021
@kyleheadley
Copy link
Member Author

which would pull in the LLVM 12 upgrade, among other things

Yes, this is why the merge was partial (and re-considered draft), as not all my equipment is yet capable of supporting the upgrade. I'll be happy for the help when the time comes, or earlier if the upgrades are expected to be trivial.

@kyleheadley
Copy link
Member Author

I'm looking at a tricky setup revealed by parsons benchmark. Here are my test cases:

// Code reduced from parsons

// stand in for system free
_Itype_for_any(T) void free(void *free_ptr : itype(_Ptr<T>));

// can force functions to be wild
void extern_fp((*free_fun)(void*));

// our nested generics example - wrap_free is inferred and free is itype
// wrap_free is not made generic because of unsafe use
static void wrap_free(void *wrap_ptr) {
  free(wrap_ptr);
}

// use fp to set function wild
void make_wild(void) {
  extern_fp(wrap_free);
}



// Basic functionality

// inferred generic
void viewer(void *i) { return; }
// our nested generics example - both inferred, everything safe
void call_from_gen_fn(void *i) {
  // CHECK: _For_any(T) void call_from_gen_fn(_Ptr<T> i) {
  viewer(i);
  // CHECK: viewer<T>(i);
}

Both of these deal with nested generics along with inferred generics. The type arguments are constraints from the local code, which may be safe or wild determined after constraint resolution.The parameter coming from the outer function may or may not be generic, to be determined after constraint resolution. But we don't propagate generic information through constraints, so a type parameter has two constraints, one generated at the call site to propagate wildness, and another for generics. This is not ideal, but the best option for now. The constraint holding the generics info must be the argument to the function that is also a parameter from the outer function. That way if it the function changes it to generic, that info is passed to the call site. Otherwise, nested generics are not supported. I need to come up with a non-supported example to make sure it is at least handled reasonably.

In the second example, The type parameter to viewer must come from an inferred generic function. This means that the initial type is void* and may lead to it being considered wild. Currently, it is marked wild (to the detriment of root cause analysis), but that wildness has no affect on the type variable because there is a good candidate for generics info (i) that is used to generate the type parameter instead. But this may have to change because of the first example and root cause cleanup.

In the first example, the call to free is within a potentially-generic function. But that function is made wild through the use of it as a parameter in an external function. Because there is a good candidate for generics info in the call to free, that constraint (of wrap_ptr) is seen, recognized as potentially generic, and rewrites a "T" as the type argument to free even though it is not used in the outer function wrap_free. That suggests that potential generics are not reset after constraint generation and should be. Or that we could check to see if the potential generic ends up wild and therefor not generic.

This analysis was written after a few false starts and may not be complete, but hopefully helps people understand. It has also helped me identify specific next steps.

@kyleheadley
Copy link
Member Author

Fix for the above situation with nested generics was to check if the generic constraint was wild, not the generated one, and to use it instead if it is safe. If the generic constraint isn't used, the generated one will go through existing wildness checks anyway.

mattmccutchen-cci added a commit that referenced this pull request Aug 17, 2021
@kyleheadley
Copy link
Member Author

Moved to a new branch and PR because this one was old and needed a significant merge with main. The new one is here

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

Successfully merging this pull request may close these issues.

3 participants