================
@@ -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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
