[PATCH] D70606: LLD: CET shadow stack support on Windows

[Petr Penzin via Phabricator via llvm-commits]

penzn added inline comments.


================
Comment at: lld/MinGW/Options.td:105
 def: F<"nxcompat">;
+def: F<"cetcompat">;
 def: F<"pic-executable">;
----------------
mstorsjo wrote:
> penzn wrote:
> > mstorsjo wrote:
> > > FWIW, if you want the option to actually have any effect when passed to the mingw driver, you need a couple lines in MinGW/Driver.cpp (and a corresponding test in test/MinGW/driver.test).
> > Good point, thank you. That is intentional - I noticed that other MSVC security flags have no effect in MinGW driver.
> > 
> > There is a patch for GNU-compatible linker on Linux, [[ https://reviews.llvm.org/D59780 | D59780 ]], though I am not sure if we can simply wire those options to COFF implementation introduced here. The goal of this patch is to support the VS-compatible flag.
> > 
> > I might be wrong with my assumptions on MinGW driver, any clarification would be greatly appreciated.
> Well, some of them, like nxcompat, are enabled by default within lld, and I'm not sure there's any GNU linker option to disable it, so therefore we don't actually act upon that option, while other flags like dynamicbase do exist and are forwarded.
> 
> As there's no predecent for these options being used in the wild with GNU ld for mingw so far, you could just omit it from this file if you aren't planning on hooking it up, as it won't be necessary for compatibility with existing projects built for mingw.
> 
> But I presume it could make sense to enable this flag in mingw configurations as well (would it?), and in that case, hooking it up with the same option name as for the ELF frontend, forwarding it to the link/COFF style flag name would be most welcome.
GNU would have its own CET flags, there is an effort to port them to LLD in [[ https://reviews.llvm.org/D59780 | D59780 ]]. I can remove the flag from MinGW to avoid confusion, though I don't feel strongly either way. What do you think?


================
Comment at: llvm/test/tools/llvm-readobj/coff-cet-compat.test:10-14
+CHECK:    AddressOfRawData: 0x146A8
+CHECK:    PointerToRawData: 0x138A8
+CHECK:    ExtendedCharacteristics [ (0x1)
+CHECK:      IMAGE_DLL_CHARACTERISTICS_EX_CET_COMPAT (0x1)
+CHECK:    ]
----------------
ruiu wrote:
> It looks like there are still a few unused bit in the real DLLCharacteristics bit (https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#dll-characteristics), so it is a bit odd that Microsoft chose to define an "extended" DLLCharacteristics field now, but well, if they chose this format we'll have to follow.
> 
> Are AddressOfRawData and PointerToRawData significant for ExtendedCharacteristics entries? I'd b  tempted to fill them with zero, as they don't seem to make any meaning in this context.
Yep, I am also not sure why this was necessary either, may be they are seeing some important use for the new field (right now it a can only contain this bit).

AddressOfRawData and PointerToRawData are coming from MSVC linker, I think that is where the bit is stored. I don't know if it is possible to "inline" it into the entry somehow:

> If the Type field is set to IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS, the debug raw data contains extended DLL characteristics bits. [...]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70606