[OpenCL] Check for extension string extension lookup

[erik2020]

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.

[Anastasia]

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

[erik2020]

In D90928#2379322, @Anastasia wrote:

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).

[Anastasia]

In D90928#2382111, @erik2020 wrote:

In D90928#2379322, @Anastasia wrote:

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]

Changed asserting code to return false instead.

[Anastasia]

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

[erik2020]

In D90928#2405796, @Anastasia wrote:

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.

[Anastasia]

In D90928#2408249, @erik2020 wrote:

In D90928#2405796, @Anastasia wrote:

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

47

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]

clang/include/clang/Basic/OpenCLOptions.h

47

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]

clang/include/clang/Basic/OpenCLOptions.h

47

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.

[erik2020]

Added doxygen comments to updated functions

[Anastasia]

Great! LGTM! Thanks!

clang/include/clang/Basic/OpenCLOptions.h

class OpenCLOptions {
   };
   llvm::StringMap<Info> OptMap;
 public:
   bool isKnown(llvm::StringRef Ext) const {
     return OptMap.find(Ext) != OptMap.end();
   }
 
   bool isEnabled(llvm::StringRef Ext) const {
+    assert(OptMap.find(Ext) != OptMap.end() && "Unknown extension.");
     return OptMap.find(Ext)->second.Enabled;
   }
 
   // Is supported as either an extension or an (optional) core feature for
   // OpenCL version \p CLVer.
   bool isSupported(llvm::StringRef Ext, const LangOptions &LO) const {
+    assert(OptMap.find(Ext) != OptMap.end() && "Unknown extension.");
     // In C++ mode all extensions should work at least as in v2.0.
     auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion;
     auto I = OptMap.find(Ext)->getValue();
     return I.Supported && I.Avail <= CLVer;
   }
 
   // Is supported (optional) OpenCL core features for OpenCL version \p CLVer.
   // For supported extension, return false.
   bool isSupportedCore(llvm::StringRef Ext, const LangOptions &LO) const {
+    assert(OptMap.find(Ext) != OptMap.end() && "Unknown extension.");
     // In C++ mode all extensions should work at least as in v2.0.
     auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion;
     auto I = OptMap.find(Ext)->getValue();
     return I.Supported && I.Avail <= CLVer && I.Core != ~0U && CLVer >= I.Core;
   }
 
   // Is supported OpenCL extension for OpenCL version \p CLVer.
   // For supported (optional) core feature, return false.
   bool isSupportedExtension(llvm::StringRef Ext, const LangOptions &LO) const {
+    assert(OptMap.find(Ext) != OptMap.end() && "Unknown extension.");
     // In C++ mode all extensions should work at least as in v2.0.
     auto CLVer = LO.OpenCLCPlusPlus ? 200 : LO.OpenCLVersion;
     auto I = OptMap.find(Ext)->getValue();
     return I.Supported && I.Avail <= CLVer && (I.Core == ~0U || CLVer < I.Core);
   }
 
   void enable(llvm::StringRef Ext, bool V = true) {
     OptMap[Ext].Enabled = V;