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

[pengfei]

Summary

This is MSVC's behaviour. LLD was matching it before D99078. Let's go back this way.

[alvinhochun]

lld/COFF/DriverUtils.cpp

109–110

Since ehcont implies cf, perhaps in this case it should also imply longjmp?

[pengfei]

lld/COFF/DriverUtils.cpp

109–110

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!

[rnk]

lld/COFF/DriverUtils.cpp

109–110

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.

[pengfei]

lld/COFF/DriverUtils.cpp

109–110

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.

[alvinhochun]

I did a non-exhaustive test with clang++ ehcont_guard_msvc.cpp -o ehcont_guard_msvc.exe -target x86_64-pc-windows-msvc -Xclang -ehcontguard -O -Xlinker /guard:xxxxxx, using MSVC Incremental Linker Version 14.29.30146.0, then checked using llvm-readobj --coff-load-config ehcont_guard_msvc.exe.

The result is a bit unexpected:

link flag	guard fid	guard longjmp	guard ehcont
(no /guard)	no	no	no
/guard:no	no	no	no
/guard:cf	yes	yes	no
/guard:no,cf	yes	yes	no
/guard:longjmp	yes	no	no
/guard:cf,longjmp	yes	yes	no
/guard:cf,nolongjmp	yes	no	no
/guard:nolongjmp,cf	yes	yes	no
/guard:ehcont	yes	no	yes
/guard:cf,ehcont	yes	yes	yes
/guard;cf,nolongjmp,ehcont	yes	no	yes
/guard:cf,longjmp,ehcont	yes	yes	yes
/guard:longjmp,ehcont	yes	no	yes
/guard:cf,longjmp,ehcont,no	no	no	no
/guard:no,cf,longjmp,ehcont	yes	yes	yes

• Options are parsed in order, negation does not override later options.
• longjmp does not actually enable guard long jump (might be a bug?)
• ehguard does indeed not enable guard long jump.

This is the code I used:

#include <cstdio>
#include <setjmp.h>

struct my_exception_t {
    int code;
};

__attribute__((noinline))
void do_test(bool should_throw) {
    printf("Starting test, should_throw = %d\n", should_throw);
    try {
        if (should_throw) {
            throw my_exception_t { 1234 };
        }
        puts("Exception not thrown.");
    } catch (my_exception_t ex) {
        printf("Caught exception with value %d.\n", ex.code);
    }
}

int main(int argc, char *argv[]) {
    if (argc > 1) {
        do_test(true);
    } else {
        do_test(false);
    }
    jmp_buf my_jmp_buf;
    jmp_buf my_jmp_buf2;
    if (setjmp(my_jmp_buf) > 0) {
        puts("Longjmp target reached.");
        return 0;
    }
    puts("Performing longjmp...");
    longjmp(my_jmp_buf2, 1);
    puts("Unexpectedly returned from longjmp!");
    return 0;
}

[pengfei]

Great job! Thanks @alvinhochun !

If we consider the longjmp not enabling long jump is a bug, and fix the table accordingly, then it is exactly what LLD is doing with this patch. It looks to me MSVC handles the options in order too. Anyway, it matches my understanding in options parsing.

[pengfei]

rebase.

[mstorsjo]

In D132901#3760680, @pengfei wrote:

Great job! Thanks @alvinhochun !

If we consider the longjmp not enabling long jump is a bug, and fix the table accordingly, then it is exactly what LLD is doing with this patch. It looks to me MSVC handles the options in order too. Anyway, it matches my understanding in options parsing.

@alvinhochun Does this patch seem ok to you in this form, or do you think it would make sense to match link.exe's behaviour wrt longjmp more exactly?

[alvinhochun]

In D132901#3776411, @mstorsjo wrote:

In D132901#3760680, @pengfei wrote:

Great job! Thanks @alvinhochun !

If we consider the longjmp not enabling long jump is a bug, and fix the table accordingly, then it is exactly what LLD is doing with this patch. It looks to me MSVC handles the options in order too. Anyway, it matches my understanding in options parsing.

@alvinhochun Does this patch seem ok to you in this form, or do you think it would make sense to match link.exe's behaviour wrt longjmp more exactly?

I think this patch in its current form is probably fine but honestly I don't know. It would be nice to have a second opinion from someone who actually use the MSVC toolchain.

[rnk]

I mentioned earlier I think this is fine as is, so I'll go ahead and mark it so in Phab.

[pengfei]

Thanks all for review!

lld/COFF/DriverUtils.cpp

void parseGuard(StringRef fullArg) {
   fullArg.split(splitArgs, ",");
   for (StringRef arg : splitArgs) {
     if (arg.equals_insensitive("no"))
       config->guardCF = GuardCFLevel::Off;
     else if (arg.equals_insensitive("nolongjmp"))
       config->guardCF &= ~GuardCFLevel::LongJmp;
     else if (arg.equals_insensitive("noehcont"))
       config->guardCF &= ~GuardCFLevel::EHCont;
-    else if (arg.equals_insensitive("cf"))
-      config->guardCF = GuardCFLevel::CF;
-    else if (arg.equals_insensitive("longjmp"))
+    else if (arg.equals_insensitive("cf") || arg.equals_insensitive("longjmp"))
       config->guardCF |= GuardCFLevel::CF | GuardCFLevel::LongJmp;
     else if (arg.equals_insensitive("ehcont"))
       config->guardCF |= GuardCFLevel::CF | GuardCFLevel::EHCont;
     else
       fatal("invalid argument to /guard: " + arg);
   }
 }
 
 // Parses a string in the form of "<subsystem>[,<integer>[.<integer>]]".
 void parseSubsystem(StringRef arg, WindowsSubsystem *sys, uint32_t *major,
                     uint32_t *minor, bool *gotVersion) {

lld/test/COFF/gfids-corrupt.s

 # REQUIRES: x86
 # RUN: llvm-mc -triple x86_64-windows-msvc %s -filetype=obj -o %t.obj
-# RUN: lld-link %t.obj -opt:noref -guard:cf -out:%t.exe -entry:main 2>&1 | FileCheck %s --check-prefix=ERRS
+# RUN: lld-link %t.obj -opt:noref -guard:cf,nolongjmp -out:%t.exe -entry:main 2>&1 | FileCheck %s --check-prefix=ERRS
 # RUN: llvm-readobj --file-headers --coff-load-config %t.exe | FileCheck %s
 
 # ERRS: warning: ignoring .gfids$y symbol table index section in object {{.*}}gfids-corrupt{{.*}}
 # ERRS: warning: ignoring invalid symbol table index in section .gfids$y in object {{.*}}gfids-corrupt{{.*}}
 
 # The table is arbitrary, really.
 # CHECK: ImageBase: 0x140000000
 # CHECK: LoadConfig [

lld/test/COFF/gfids-fallback.s

 # REQUIRES: x86
 # RUN: grep -B99999 [S]PLITMARKER %s | llvm-mc -triple x86_64-windows-msvc -filetype=obj -o %t1.obj
 # RUN: grep -A99999 [S]PLITMARKER %s | llvm-mc -triple x86_64-windows-msvc -filetype=obj -o %t2.obj
-# RUN: lld-link %t1.obj %t2.obj -guard:cf -out:%t.exe -entry:main -opt:noref
+# RUN: lld-link %t1.obj %t2.obj -guard:cf,nolongjmp -out:%t.exe -entry:main -opt:noref
 # RUN: llvm-readobj --file-headers --coff-load-config %t.exe | FileCheck %s
 
 # CHECK: ImageBase: 0x140000000
 # CHECK: LoadConfig [
 # CHECK:   SEHandlerTable: 0x0
 # CHECK:   SEHandlerCount: 0
 # CHECK:   GuardCFCheckFunction: 0x0
 # CHECK:   GuardCFCheckDispatch: 0x0

lld/test/COFF/gfids-gc.s

 # REQUIRES: x86
 # RUN: llvm-mc -triple x86_64-windows-msvc %s -filetype=obj -o %t.obj
-# RUN: lld-link %t.obj -guard:cf -out:%t.exe -opt:noref -entry:main
+# RUN: lld-link %t.obj -guard:cf,nolongjmp -out:%t.exe -opt:noref -entry:main
 # RUN: llvm-readobj --file-headers --coff-load-config %t.exe | FileCheck %s --check-prefix=CHECK-NOGC
-# RUN: lld-link %t.obj -guard:cf -out:%t.exe -opt:noref -entry:main -debug:dwarf
+# RUN: lld-link %t.obj -guard:cf,nolongjmp -out:%t.exe -opt:noref -entry:main -debug:dwarf
 # RUN: llvm-readobj --file-headers --coff-load-config %t.exe | FileCheck %s --check-prefix=CHECK-NOGC
-# RUN: lld-link %t.obj -guard:cf -out:%t.exe -opt:ref -entry:main
+# RUN: lld-link %t.obj -guard:cf,nolongjmp -out:%t.exe -opt:ref -entry:main
 # RUN: llvm-readobj --file-headers --coff-load-config %t.exe | FileCheck %s --check-prefix=CHECK-GC
 
 # This assembly is meant to mimic what CL emits for this kind of C code when
 # /Gw (-fdata-sections) is enabled:
 # int f() { return 42; }
 # int g() { return 13; }
 # int (*fp1)() = &f;
 # int (*fp2)() = &g;

lld/test/COFF/gfids-icf.s

 # REQUIRES: x86
 # RUN: llvm-mc -triple x86_64-windows-msvc %s -filetype=obj -o %t.obj
-# RUN: lld-link %t.obj -guard:cf -out:%t.exe -opt:icf -entry:main
+# RUN: lld-link %t.obj -guard:cf,nolongjmp -out:%t.exe -opt:icf -entry:main
 # RUN: llvm-readobj --file-headers --coff-load-config %t.exe | FileCheck %s --check-prefix=CHECK
 
 # This assembly is meant to mimic what CL emits for this kind of C code:
 # int icf1() { return 42; }
 # int icf2() { return 42; }
 # int (*fp1)() = &icf1;
 # int (*fp2)() = &icf2;
 # int main() {

lld/test/COFF/guard-longjmp.s

 # REQUIRES: x86
 # RUN: llvm-mc -triple x86_64-windows-msvc %s -filetype=obj -o %t.obj
-# RUN: lld-link %t.obj -guard:cf -guard:longjmp -out:%t.exe -entry:main
+# RUN: lld-link %t.obj -guard:cf -out:%t.exe -entry:main
 # RUN: llvm-readobj --file-headers --coff-load-config %t.exe | FileCheck %s
 
 # CHECK: ImageBase: 0x140000000
 # CHECK: LoadConfig [
 # CHECK:   SEHandlerTable: 0x0
 # CHECK:   SEHandlerCount: 0
 # CHECK:   GuardCFCheckFunction: 0x0
 # CHECK:   GuardCFCheckDispatch: 0x0

lld/test/COFF/guardcf-lto.ll

 ; REQUIRES: x86
 ; Set up an import library for a DLL that will do the indirect call.
 ; RUN: echo -e 'LIBRARY library\nEXPORTS\n  do_indirect_call\n' > %t.def
 ; RUN: lld-link -def:%t.def -out:%t.lib -machine:x64
 
 ; Generate an object that will have the load configuration normally provided by
 ; the CRT.
 ; RUN: llvm-mc -triple x86_64-windows-msvc -filetype=obj %S/Inputs/loadconfig-cfg-x64.s -o %t.ldcfg.obj
 
 ; RUN: llvm-as %s -o %t.bc
-; RUN: lld-link -entry:main -guard:cf -guard:longjmp -dll %t.bc %t.lib %t.ldcfg.obj -out:%t2.dll
+; RUN: lld-link -entry:main -guard:cf -dll %t.bc %t.lib %t.ldcfg.obj -out:%t2.dll
 ; RUN: llvm-readobj --coff-load-config %t2.dll | FileCheck %s
 
 ; There must be *two* entries in the table: DLL entry point, and my_handler.
 
 ; CHECK:      LoadConfig [
 ; CHECK:        GuardCFFunctionTable: 0x{{[^0].*}}
 ; CHECK-NEXT:   GuardCFFunctionCount: 2
 ; CHECK-NEXT:   GuardFlags [ (0x10500)