[PATCH] D132901: [LLD] Imply "longjmp" in `/guard:cf`

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: lld/COFF/DriverUtils.cpp:109-110
       config->guardCF |= GuardCFLevel::CF | GuardCFLevel::LongJmp;
     else if (arg.equals_insensitive("ehcont"))
       config->guardCF |= GuardCFLevel::CF | GuardCFLevel::EHCont;
     else
----------------
rnk wrote:
> pengfei wrote:
> > alvinhochun wrote:
> > > Since `ehcont` implies `cf`, perhaps in this case it should also imply `longjmp`?
> > No idea. But if we make `ehcont` imply `longjmp`, an option like `-guard:cf,nolongjmp,ehcont` won't disable `nolongjmp`. So it should align with MSVC too. Could you help to have a check? Thanks!
> I think to align with MSVC we'd have to change the code structure to process negation options after positive options, rather than updating the feature mask in order. We would also need to check the semantics of `/guard:longjump,nolonjump` / `/guard:nolongump,longjump`, those probably become warnings or errors.
> 
> Attention to detail like this is appreciated, but I'd also approve this change as is to get us back to where we were.
Yeah. Option consistency is important, however, an explicit or implicit confliction option is in the grey area.
AFAIK, Clang driver always overrides former by latter without warning or error. Does MSVC take precedence on negation options?
I'm not a frequent user of MSVC. So I'm not sure if MSVC is constant in such "UB" options or if we are overengineering on mocking it.
I'd like to be conservative here, i.e., not let `ehcont` imply `longjmp`, given I don't have environment to verify it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132901