This is an archive of the discontinued LLVM Phabricator instance.

[IR] Make stack protector symbol dso_local according to -f[no-]direct-access-external-data
ClosedPublic

Authored by MaskRay on May 17 2023, 10:50 PM.

Details

Summary

There are two motivations.

-fno-pic -fstack-protector -mstack-protector-guard=global created
__stack_chk_guard is referenced directly on all ELF OSes except FreeBSD.
This patch allows referencing the symbol indirectly with
-fno-direct-access-external-data.

Some Linux kernel folks want
-fno-pic -fstack-protector -mstack-protector-guard-reg=gs -mstack-protector-guard-symbol=__stack_chk_guard
created __stack_chk_guard to be referenced directly, avoiding
R_X86_64_REX_GOTPCRELX (even if the relocation may be optimized out by the linker).
https://github.com/llvm/llvm-project/issues/60116
Why they need this isn't so clear to me.


Add module flag "direct-access-external-data" and set the dso_local property of
the stack protector symbol. The module flag can benefit other LLVMCodeGen
synthesized symbols that are not represented in LLVM IR.

Nowadays, with -fno-pic being uncommon, ideally we should set
"direct-access-external-data" when it is true. However, doing so would require
~90 clang/test tests to be updated, which are too much.

As a compromise, we set "direct-access-external-data" only when it's different
from the implied default value.

Diff Detail

Event Timeline

MaskRay created this revision.May 17 2023, 10:50 PM
MaskRay requested review of this revision.May 17 2023, 10:50 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 17 2023, 10:50 PM
nickdesaulniers accepted this revision.May 22 2023, 3:29 PM

This also resolves https://github.com/llvm/llvm-project/issues/62482; was that intentional? If so, consider adding a link to https://github.com/llvm/llvm-project/issues/62482 to this commit description (or closing https://github.com/llvm/llvm-project/issues/62482 as a duplicate of https://github.com/llvm/llvm-project/issues/60116 if that makes more sense).

llvm/lib/IR/Module.cpp
675–684

I think the presubmit bot failed due to clang-format changes to code introduced in this hunk.

This revision is now accepted and ready to land.May 22 2023, 3:29 PM

This also resolves https://github.com/llvm/llvm-project/issues/62482; was that intentional? If so, consider adding a link to https://github.com/llvm/llvm-project/issues/62482 to this commit description (or closing https://github.com/llvm/llvm-project/issues/62482 as a duplicate of https://github.com/llvm/llvm-project/issues/60116 if that makes more sense).

Thanks for the review!

I think https://github.com/llvm/llvm-project/issues/62482 and https://github.com/llvm/llvm-project/issues/60116 are about the same thing and I've closed the former as a duplicate of the latter.

MaskRay updated this revision to Diff 524768.May 23 2023, 9:39 AM
MaskRay marked an inline comment as done.

fix clang-format lint

This revision was landed with ongoing or failed builds.May 23 2023, 9:50 AM
This revision was automatically updated to reflect the committed changes.