[PATCH] D138747: [Support] On Windows 11, fix an affinity mask issue on large core count machines

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: llvm/lib/Support/Windows/Process.inc:485-487
     auto getVer = (RtlGetVersionPtr)::GetProcAddress(hMod, "RtlGetVersion");
-    if (getVer) {
-      RTL_OSVERSIONINFOEXW info{};
-      info.dwOSVersionInfoSize = sizeof(info);
-      if (getVer((PRTL_OSVERSIONINFOW)&info) == STATUS_SUCCESS) {
-        return llvm::VersionTuple(info.dwMajorVersion, info.dwMinorVersion, 0,
-                                  info.dwBuildNumber);
-      }
-    }
-  }
-  return llvm::VersionTuple(0, 0, 0, 0);
+    if (!getVer)
+      return std::nullopt;
----------------
`RtlGetVersion` came with Windows 2000, which is older than the oldest version of Windows we support. I think we can safely assert that this interface is loaded.


================
Comment at: llvm/lib/Support/Windows/Process.inc:491-493
+    if (getVer((PRTL_OSVERSIONINFOW)&info) != STATUS_SUCCESS)
+      return std::nullopt;
+    return info;
----------------
According to MSDN, this API only returns `STATUS_SUCCESS` (https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-rtlgetversion), so I wonder if we can get away with an assert here as well, and then we don't have to worry about this interface ever resulting in no valid version information, which can propagate to the callers.


================
Comment at: llvm/lib/Support/Windows/Process.inc:502
+  if (!info)
+    return llvm::VersionTuple(0, 0, 0, 0);
+
----------------
aganea wrote:
> tschuett wrote:
> > This looks like cute way to model `nullopt`. Would returning `std::option<lvm::VersionTuple>` be an improvement?
> I made the changes so you can see how it looks from the caller perspective. I think `std::option` internally in `Process.inc` makes sense. However externally, returning `std::option` from `llvm::GetWindowsOSVersion()` leaks the fact that the Win32 APIs can fail, which clients don't really care about, and requires a bit more client-side code. Semantically clients only need to compare < or > against a version number, and 0 is fine. Please let me know your thoughts.
>  I think std::option internally in Process.inc makes sense. 

Agreed, I think it's an improvement.

> However externally, returning std::option from llvm::GetWindowsOSVersion() leaks the fact that the Win32 APIs can fail, which clients don't really care about, and requires a bit more client-side code. Semantically clients only need to compare < or > against a version number, and 0 is fine. Please let me know your thoughts.

I agree; the very first thing I did on this review was add a comment to that declaration asking for comments explaining when the result might not exist and how callers should react in that case. I'm not 100% sure we can remove the optionality from the return type, but I found some hints that suggest we can explore it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138747/new/

https://reviews.llvm.org/D138747