[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

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]

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]

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.

[erik2020]

Added doxygen comments to updated functions

[Anastasia]

Great! LGTM! Thanks!

clang/include/clang/Basic/OpenCLOptions.h

struct Info {
     unsigned Avail; // Option starts to be available in this OpenCL version
     unsigned Core;  // Option becomes (optional) core feature in this OpenCL
                     // version
     Info(bool S = false, bool E = false, unsigned A = 100, unsigned C = ~0U)
       :Supported(S), Enabled(E), Avail(A), Core(C){}
   };
   llvm::StringMap<Info> OptMap;
 public:
+  /// Check if \c Ext is a recognized OpenCL extension.
+  ///
+  /// \param Ext - Extension to look up.
+  /// \returns \c true if \c Ext is known, \c false otherwise.
   bool isKnown(llvm::StringRef Ext) const {
     return OptMap.find(Ext) != OptMap.end();
   }
 
+  /// Check if \c Ext is an enabled OpenCL extension.
+  ///
+  /// \param Ext - Extension to look up.
+  /// \returns \c true if \c Ext is known and enabled, \c false otherwise.
   bool isEnabled(llvm::StringRef Ext) const {
-    return OptMap.find(Ext)->second.Enabled;
+    auto E = OptMap.find(Ext);
+    return E != OptMap.end() && E->second.Enabled;
   }
 
-  // Is supported as either an extension or an (optional) core feature for
-  // OpenCL version \p CLVer.
+  /// Check if \c Ext is supported as either an extension or an (optional) core
+  /// feature for the given OpenCL version.
+  ///
+  /// \param Ext - Extension to look up.
+  /// \param LO - \c LangOptions specifying the OpenCL version.
+  /// \returns \c true if \c Ext is known and supported, \c false otherwise.
   bool isSupported(llvm::StringRef Ext, const LangOptions &LO) const {
+    auto E = OptMap.find(Ext);
+    if (E == OptMap.end()) {
+      return false;
+    }
     // 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();
+    auto I = E->getValue();
     return I.Supported && I.Avail <= CLVer;
   }
 
-  // Is supported (optional) OpenCL core features for OpenCL version \p CLVer.
-  // For supported extension, return false.
+  /// Check if \c Ext is supported as an (optional) OpenCL core features for
+  /// the given OpenCL version.
+  ///
+  /// \param Ext - Extension to look up.
+  /// \param LO - \c LangOptions specifying the OpenCL version.
+  /// \returns \c true if \c Ext is known and supported, \c false otherwise.
   bool isSupportedCore(llvm::StringRef Ext, const LangOptions &LO) const {
+    auto E = OptMap.find(Ext);
+    if (E == OptMap.end()) {
+      return false;
+    }
     // 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();
+    auto I = E->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.
+  /// Check if \c Ext is a supported OpenCL extension for the given OpenCL
+  /// version.
+  ///
+  /// \param Ext - Extension to look up.
+  /// \param LO - \c LangOptions specifying the OpenCL version.
+  /// \returns \c true if \c Ext is known and supported, \c false otherwise.
   bool isSupportedExtension(llvm::StringRef Ext, const LangOptions &LO) const {
+    auto E = OptMap.find(Ext);
+    if (E == OptMap.end()) {
+      return false;
+    }
     // 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();
+    auto I = E->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;
   }
 
   /// Enable or disable support for OpenCL extensions