Page MenuHomePhabricator

[CodeGen] Restore accessing __stack_chk_guard via a .refptr stub on mingw after 2518433f861fcb87
ClosedPublic

Authored by mstorsjo on Dec 6 2020, 12:52 PM.

Details

Reviewers
MaskRay
rnk
Summary

Add tests for this particular detail for x86 and arm (similar tests already existed for x86_64 and aarch64).

The libssp implementation may be located in a separate DLL, and in those cases, the references need to be in a .refptr stub, to avoid needing to touch up code in the text section at runtime (which is supported but inefficient for x86, and unsupported for arm).

There were half a dozen of commits at least on top of this change (so it's not trivial to revert back to working condition, to add more comprehensive tests for the original state), and none of them seem to have an associated phabricator review thread to comment on, so commenting here instead...

Diff Detail

Unit TestsFailed

TimeTest
60 msx64 windows > LLVM.CodeGen/XCore::threads.ll
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llc.exe -march=xcore < C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll

Event Timeline

mstorsjo created this revision.Dec 6 2020, 12:52 PM
mstorsjo requested review of this revision.Dec 6 2020, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2020, 12:52 PM
MaskRay accepted this revision.EditedDec 6 2020, 7:14 PM

Sorry, I did not know Windows has ssp...

The if (getTargetMachine().getRelocationModel() == Reloc::Static) change was added to keep the current ELF behavior in static relocation model (which may cause a copy relocation if the variable is defined in a shared object (libc.so))... Restricting this to Windows looks find

The long-term plan is:

  • Add -fdirect-access-external-data to supersede -mpie-copy-relocations D92633
  • Make -fdirect-access-external-data work with -fno-pic D92714
  • Make -fdirect-access-external-data set a module flag (I don't have a good name yet): control whether default visibility declarations (both variables and functions) should default to dso_local
  • Rewrite if (isa<llvm::Function>(GV) && !CGOpts.NoPLT && RM == llvm::Reloc::Static) (https://github.com/llvm/llvm-project/blob/master/clang/lib/CodeGen/CodeGenModule.cpp#L983 ) to use the module flag (the option is called ***data, but I guess it makes sense for dso_local specifiers on function declarations as well since the dso_local specifier controls the behavior of "taking the address of an undefined function" (which is used similar to a data symbol)
  • Let Module::getOrInsertGlobal() check the module flag and make variable declarations dso_local

Actually, marking undefined function/variable declarations dso_local in ELF -fno-pic code (allow R_X86_64_64/R_386_32) has a low priority (lots of users have switched to -fpie + users don't care 32-bit (performance) much + GOTPCRELX can retrieve back most of the performance cost on 64-bit code). I just had not fixed an x86-64 FastISel bug 687b83ceabafe81970cd4639e7f0c89036402081 when I thought we should mark some declarations dso_local

This revision is now accepted and ready to land.Dec 6 2020, 7:14 PM