This is an archive of the discontinued LLVM Phabricator instance.

[X86]Support options -mno-gather -mno-scatter
ClosedPublic

Authored by XinWang10 on Aug 11 2023, 12:29 AM.

Details

Summary

Gather instructions could lead to security issues, details please refer to https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/gather-data-sampling.html.
This supported options -mno-gather and -mno-scatter, which could avoid generating gather/scatter instructions in backend except using intrinsics or inline asms.

Diff Detail

Event Timeline

XinWang10 created this revision.Aug 11 2023, 12:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2023, 12:29 AM
XinWang10 requested review of this revision.Aug 11 2023, 12:29 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 11 2023, 12:29 AM
  • restore blank line
wangpc added inline comments.Aug 11 2023, 1:36 AM
clang/lib/Driver/ToolChains/Clang.cpp
7527 ↗(On Diff #549268)

Put these in clang/lib/Driver/ToolChains/Arch/X86.cpp:getX86TargetFeatures? They are target-specific.

XinWang10 updated this revision to Diff 549337.Aug 11 2023, 4:18 AM
  • mv driver implement to x86 specific code
pengfei added inline comments.Aug 11 2023, 5:38 AM
clang/include/clang/Driver/Options.td
903–906

Move under "// X86 feature flags"

clang/lib/Driver/ToolChains/Arch/X86.cpp
272–277

No need parentheses

llvm/test/CodeGen/X86/x86-prefer-no-gather-no-scatter.ll
10–11

Don't need these and is duplicated with RUN line triple.

MaskRay added inline comments.Aug 11 2023, 7:02 AM
clang/include/clang/Driver/Options.td
903–906

Also remove NoXarchOption

https://maskray.me/blog/2021-03-28-compiler-driver-and-cross-compilation#misc

If an option has the NoXarchOption flag, ClangDriver will emit an error if the option is used after -Xarch_* (originally for universal macOS binary, reused by offloading purposes -Xarch_host/etc). The error checking only applies to a small set of options (e.g. -o) and is not very useful for most options, but NoXarchOption was improperly named DriverOption (commit aabb0b11a3c1d8a6bb859db80400cffdcc9b336f) and lured some contributors to add NoXarchOption to options that should not have the flag.

skan added inline comments.Aug 11 2023, 7:47 AM
llvm/lib/Target/X86/X86.td
437

Does "Prefer no gather instructions" sound better?

I think these two should be put under "X86 Subtarget Tuning features"?

XinWang10 updated this revision to Diff 549571.Aug 11 2023, 9:22 PM
  • resolve comments
XinWang10 marked 4 inline comments as done.Aug 11 2023, 9:24 PM
XinWang10 added inline comments.
llvm/lib/Target/X86/X86.td
437

I think the two options are to mitigate security issues. Could refer to link in summary.

pengfei added inline comments.Aug 11 2023, 10:25 PM
llvm/lib/Target/X86/X86.td
437

It depends on if the micro code was applied. We should assume user care of this option should have applied the micro code. So it's more like a performance turning rather than mitigation. And you cannot disable all gather/scatter instructions with these options.

XinWang10 marked an inline comment as done.Aug 13 2023, 7:48 PM
XinWang10 added inline comments.
llvm/lib/Target/X86/X86.td
437

Micro code applied? You mean we should keep eye on the byte code generated.
And what's the essential difference between performance turning and mitigation? Do mitigation for no-gather means we could not emit gather whenever but performance turning could exist some gather considering the performance?
Second, I think the intention for this option is to mitigate the security issue but not tune the performance.

  • mov definition to tuning features
XinWang10 marked an inline comment as done.Aug 13 2023, 11:39 PM
XinWang10 added inline comments.
llvm/lib/Target/X86/X86.td
437

Talked offline and agreed to move to performance tuning.

pengfei accepted this revision.Aug 16 2023, 1:46 AM

LGTM.

This revision is now accepted and ready to land.Aug 16 2023, 1:46 AM
Matt added a subscriber: Matt.Aug 16 2023, 1:14 PM
MaskRay added inline comments.Aug 16 2023, 4:26 PM
clang/test/Driver/x86-no-gather-no-scatter.cpp
4

Each RUN line makes testsuite execution slower and we should be careful adding more lines.

We don't generally additionally test %clangxx when %clang suffices. For non-linking, %clang is mostly identical to %clangxx.

17

No newline at end of file

  • update fe test
XinWang10 marked 2 inline comments as done.Aug 16 2023, 10:41 PM
thakis added a subscriber: thakis.Aug 18 2023, 6:48 AM
thakis added inline comments.
clang/test/Driver/x86-no-gather-no-scatter.cpp
4

FYI, %clang_cl always must use -- before %s, else tests fail on macOS. I fixed this test in 547ee1c81fceaabcb.