Page MenuHomePhabricator

[OpenCL] Check for extension string extension lookup
ClosedPublic

Authored by erik2020 on Nov 6 2020, 3:34 AM.

Details

Summary

Calling any of the OpenCLOptions::is*() functions (except isKnown()) with an unknown extension string results in a seg fault. This patch checks that the extension exists in the map before attempting to access it.

Diff Detail

Event Timeline

erik2020 created this revision.Nov 6 2020, 3:34 AM
erik2020 requested review of this revision.Nov 6 2020, 3:34 AM

Ok, it would still segfault but perhaps it is acceptable as this is an internal frontend only option for now.

Ok, it would still segfault but perhaps it is acceptable as this is an internal frontend only option for now.

Would it be better if these functions returned false for unknown extensions? I think it would be consistent with the function names (e.g., isEnabled() returns false for an unknown extensions, because an unknown extension cannot be enabled).

Ok, it would still segfault but perhaps it is acceptable as this is an internal frontend only option for now.

Would it be better if these functions returned false for unknown extensions? I think it would be consistent with the function names (e.g., isEnabled() returns false for an unknown extensions, because an unknown extension cannot be enabled).

Yes, this makes more sense indeed.

erik2020 updated this revision to Diff 306094.Nov 18 2020, 6:50 AM

Changed asserting code to return false instead.

erik2020 retitled this revision from [OpenCL] Add assertions to extension lookup to [OpenCL] Check for extension string extension lookup.Nov 18 2020, 6:51 AM
erik2020 edited the summary of this revision. (Show Details)

LGTM for the fix! Do you think we could improve testing? I presume there is something that triggers a failure without your change...

Do you think we could improve testing? I presume there is something that triggers a failure without your change...

I'm not really sure how to test this code. Best I can tell, there's no way for the clang executable to call these functions with invalid strings. I only ran into the seg faults when I was programmatically setting options using the clang API.

Do you think we could improve testing? I presume there is something that triggers a failure without your change...

I'm not really sure how to test this code. Best I can tell, there's no way for the clang executable to call these functions with invalid strings. I only ran into the seg faults when I was programmatically setting options using the clang API.

Oh, I see. We could also create a g-test for this but it's probably not worth enough. Perhaps if you just update a comment that the API behavior becomes clear it should be good enough.

clang/include/clang/Basic/OpenCLOptions.h
59

Btw how about we use isKnown instead because it does exactly that? Also, I think we should update the comment to explain this change in the API behavior and add a comment for isKnown.

erik2020 added inline comments.Nov 23 2020, 9:41 AM
clang/include/clang/Basic/OpenCLOptions.h
59

My thinking was that this way, there's only one look-up in OptMap, but when using isKnown() there are two. I don't know how good a compiler would be at inlining isKnown() and de-duplicating the look-ups.

But maybe the overhead doesn't really matter in this case and calling isKnown() is clearer?

Anastasia added inline comments.Nov 23 2020, 11:03 AM
clang/include/clang/Basic/OpenCLOptions.h
59

I would say it is likely to be inlined in the optimized builds, removing duplicate calls to find would be trickier but since it's part of the standard libraries I think it is very likely going to happen.

Ok, if you are concerned about the performance we can leave it as is and only update the comments.

Anastasia removed a subscriber: Anastasia.
erik2020 updated this revision to Diff 307338.Nov 24 2020, 6:52 AM

Added doxygen comments to updated functions

Anastasia accepted this revision.Nov 26 2020, 9:13 AM

Great! LGTM! Thanks!

This revision is now accepted and ready to land.Nov 26 2020, 9:13 AM
This revision was automatically updated to reflect the committed changes.