[clang] [clang] fix uefi target for aarch64 & x86_64 (PR #120632)

[Roland McGrath via cfe-commits]

================
@@ -713,8 +713,8 @@ class LLVM_LIBRARY_VISIBILITY X86_64TargetInfo : public X86TargetInfo {
   X86_64TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
       : X86TargetInfo(Triple, Opts) {
     const bool IsX32 = getTriple().isX32();
-    bool IsWinCOFF =
-        getTriple().isOSWindows() && getTriple().isOSBinFormatCOFF();
+    bool IsWinCOFF = (getTriple().isOSWindows() || getTriple().isUEFI()) &&
+                     getTriple().isOSBinFormatCOFF();
     LongWidth = LongAlign = PointerWidth = PointerAlign = IsX32 ? 32 : 64;
----------------
frobtech wrote:

> @frobtech Yeah, that's kinda why I am unsure of the changes suggested.

Here AFAICT `IsWinCOFF` is being used only to choose the [datalayout string](https://llvm.org/docs/LangRef.html#langref-datalayout).  The only difference I see is `m:w` vs `m:e`, which is about assembler symbol naming details.  That's about the rules and conventions for PE-COFF compilers, assemblers, and linkers. This one case I don't think is something where UEFI users would ever need an option to differ from Windows users. There's probably only one right way to interface with the linker.

It seems to be called `IsWinCOFF` mainly because the way it's been checked for is "is COFF and is Windows".  I suspect that "is COFF" is true for other forms of COFF that aren't PE-COFF, but there isn't an `isOSBinFormatPECOFF()` that distinguishes, so this has been used as a proxy without explanation.  For this case, `IsPECOFF` would be better name locally, anyway, to express what it is that matters to the check.

IMHO it would be wisest *not* to go around making lots of things write out `...isOsWindows() || ...isOsUEFI()`.  Instead there should be an `.IsOSBinFormatPECOFF()` or the like (probably written as just that same OR anyway), where it's clearly expressed in the name of the call what property it is that is the determinant for each use case.

Just in this same file there are other local variables called `IsWinCOFF`, presumably likewise named because they are set the same way rather than clearly expressing what they are really checking for.  But that other use is do set `MaxVectorAlign`, which seems a lot like something that's really about runtime ABI subtleties, perhaps such as the expectations about stack and heap alignment.  Something like that very well may more properly be for Windows (and I don't know why it's for Windows&&COFF instead of just for Windows, maybe there's a distinction), and not for "all PE-COFF" and thus not for UEFI.

https://github.com/llvm/llvm-project/pull/120632