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

Fix Issue 20426 - doesPointTo with void[N] fails #7458

Merged
merged 1 commit into from
Apr 26, 2020

Conversation

MoonlightSentinel
Copy link
Contributor

Accept a false positive/negative for may/doesPointTo

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @MoonlightSentinel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
20426 normal doesPointTo with void[N] fails

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7458"

Copy link
Member

@n8sh n8sh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If source is or contains a union, then there may be either false positives or false negatives

This PR should update the documentation to specify that false positives / false negatives are also possible if source is or contains a static array whose element type is void.

std/exception.d Outdated
Comment on lines 1197 to 1217
static if (is(S == void[n], size_t n))
{
return true;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do more than this in mayPointTo. Minimally return S.sizeof >= typeof(void*).sizeof as we know that a data chunk smaller than a pointer cannot contain a pointer. Maximally we could interpret every properly-aligned pointer-sized chunk of the static array as if it were a pointer, which is safe since we never actually dereference the pointer but merely compare it to other pointers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented the latter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Branch for CTFE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a __ctfe branch which pessimistically returns true because I didn't find a way to implement the actual logic without using a reinterpreting cast.

std/exception.d Outdated Show resolved Hide resolved
std/exception.d Outdated
(cast(void*[]) voidArr[])[] = null; // Ensure no false pointers

*cast(void**) &voidArr[16] = &a; // Pointers should be found
*cast(void**) &voidArr[3] = &b; // But only if they are properly aligned
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if misaligned writes are guaranteed to work with LDC. I guess if some supported platform actually ignores the lowest few bits of a destination address that it "knows" is aligned on a word boundary, the automated test suite will tell us.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would writing individual bytes using slice assignment work for all platforms? E.g.:

void*[1] tmp = [&b];
(cast(ubyte[]) voidArr[3 .. 3 + (void*).sizeof])[] = cast(ubyte[]) tmp[];

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely.

Accept a false positive/negative for may/doesPointTo
{
// Reinterpreting cast is impossible during ctfe
if (__ctfe)
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to make this work, you should be able to cast to some form of ubyte[], but the compiler refuses to see anything but a void[N]. I tried slicing, slicing a pointer, and using a helper function.

@dlang-bot dlang-bot merged commit b136191 into dlang:master Apr 26, 2020
@n8sh
Copy link
Member

n8sh commented Apr 26, 2020

Unfortunately there's a problem with the mayPointTo(void[n], T) check that didn't occur to me in time. Aside from containing pointers, the mystery memory region could also contain the language's built-in array slices that are represented as (length, pointer) pairs. Since we also don't know the element size, any (length, pointer) pair where the pointer is non-null and the length is non-zero potentially refers to the entire memory range from the pointer to the pointer plus the greatest multiple of the length that can be added to it without overflow.

IDK if it's worth checking this. It might be better to change mayPointTo(void[n], T) back to always returning true like in the first version of this PR.

@MoonlightSentinel MoonlightSentinel deleted the void-point branch April 26, 2020 19:39
@schveiguy
Copy link
Member

Hm... I think it's worth revising, the use case that prompted this is TaggedAlgebraic, which uses a void[N] for storage. That could easily contain an array. Given that mayPointTo returning false is supposed to be "100% sure", I'd say we should opt for the cautious version.

@schveiguy
Copy link
Member

We could still implement checks like if the N is 3, it can't be a pointer, or if it's 5, it can't be an array (on 32-bit), but I'd say that the easiest thing is to return true. It can be enhanced later if a need arises.

schveiguy added a commit to schveiguy/phobos that referenced this pull request Apr 27, 2020
schveiguy added a commit to schveiguy/phobos that referenced this pull request Apr 27, 2020
… hold a slice,

unless the entire thing is zero
schveiguy added a commit to schveiguy/phobos that referenced this pull request Apr 27, 2020
… hold a slice,

unless the entire thing is zero
schveiguy added a commit to schveiguy/phobos that referenced this pull request Apr 27, 2020
… hold a slice,

unless the entire thing is zero
schveiguy added a commit to schveiguy/phobos that referenced this pull request Apr 28, 2020
… hold a slice,

unless the entire thing is zero
schveiguy added a commit to schveiguy/phobos that referenced this pull request Apr 30, 2020
… hold a slice,

unless the entire thing is zero
schveiguy added a commit to schveiguy/phobos that referenced this pull request Apr 30, 2020
… hold a slice,

unless the entire thing is zero
dlang-bot added a commit that referenced this pull request May 4, 2020
Followup to #7458 - Always return true for mayPointTo if the void[N] …
merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>
n8sh pushed a commit to n8sh/phobos that referenced this pull request May 18, 2020
… hold a slice,

unless the entire thing is zero
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants