[PATCH] D72829: Implement -fsemantic-interposition

This revision was automatically updated to reflect the committed changes. Closed by commit rGfd09f12f32f5: Implement -fsemantic-interposition (authored by serge-sans-paille). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62325 tests passed, 0 failed and 838 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 241351. serge-sans-paille added a comment. Fix documentation ordering, thank goes to @arichardson for spotting that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/

[PATCH] D72829: Implement -fsemantic-interposition

arichardson added a comment. Thanks documentation looks much better now. Comment at: clang/docs/ClangCommandLineReference.rst:907 + +.. option:: -fsemantic-interposition, -fno-semantic-interposition + arichardson wrote: > arichardson wrote: > > This looks like

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62312 tests passed, 0 failed and 838 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 241254. serge-sans-paille added a comment. take @sfertile review into account. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/ https://reviews.llvm.org/D72829 Files:

[PATCH] D72829: Implement -fsemantic-interposition

sfertile added a comment. In D72829#1846353 , @arichardson wrote: > As this is user-facing documentation I feel like there should be a slightly > longer explaning what the option does. +1 on this, otherwise LGTM. Thanks for implementing this!

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62300 tests passed, 0 failed and 837 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille added a comment. > As this is user-facing documentation I feel like there should be a slightly > longer explaning what the option does. done! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 241152. serge-sans-paille added a comment. Update user-level documentation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/ https://reviews.llvm.org/D72829 Files:

[PATCH] D72829: Implement -fsemantic-interposition

sfertile added a comment. Aewsome, thanks for implementing this. I was on vacation for a bit and somehow missed this review in my queue when I cam back but having a look at it now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/

[PATCH] D72829: Implement -fsemantic-interposition

arichardson added a comment. In D72829#1846157 , @serge-sans-paille wrote: > @arichardson doc updated, any other advice? No comments on the code just wondering if the documentation should be expanded. I just came across this review by chance and it was

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62286 tests passed, 0 failed and 827 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 241055. serge-sans-paille added a comment. @arichardson doc updated, any other advice? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/ https://reviews.llvm.org/D72829 Files:

[PATCH] D72829: Implement -fsemantic-interposition

arichardson added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:907 + +.. option:: -fsemantic-interposition, -fno-semantic-interposition + This looks like it should be reordered? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62281 tests passed, 0 failed and 827 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72829: Implement -fsemantic-interposition

MaskRay accepted this revision. MaskRay added a comment. In D72829#1845396 , @serge-sans-paille wrote: > > I suppose you mean whether "SemanticInterposition" should be invalidated > > when "PIC Level" does not exist or "PIE Level" exists. I am a bit

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille marked an inline comment as done. serge-sans-paille added a comment. > I suppose you mean whether "SemanticInterposition" should be invalidated when > "PIC Level" does not exist or "PIE Level" exists. I am a bit inclined to make > it more flexible/orthogonal in the backend,

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 240966. serge-sans-paille marked an inline comment as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/ https://reviews.llvm.org/D72829 Files: clang/docs/ClangCommandLineReference.rst

[PATCH] D72829: Implement -fsemantic-interposition

MaskRay added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3011 + if (Args.hasArg(OPT_fsemantic_interposition) && Opts.PICLevel && !Opts.PIE) +Opts.SemanticInterposition = 1; + `Opts.SemanticInterposition` is initialized to 0 and

[PATCH] D72829: Implement -fsemantic-interposition

MaskRay added a comment. In D72829#1844050 , @serge-sans-paille wrote: > @MaskRay should we add a verifier step to check that > pie/pic/semanticinterposition module flags are consistent, or leave that to > clang ? I suppose you mean whether

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62258 tests passed, 0 failed and 827 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon times-circle color=red} clang-format: fail. Please format your changes with clang-format by running

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 240813. serge-sans-paille added a comment. @MaskRay should we add a verifier step to check that pie/pic/semanticinterposition module flags are consistent, or leave that to clang ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille marked 3 inline comments as done. serge-sans-paille added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:488 + if (Context.getLangOpts().SemanticInterposition && + (Context.getLangOpts().PICLevel && !Context.getLangOpts().PIE)) +//

[PATCH] D72829: Implement -fsemantic-interposition

MaskRay added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:488 + if (Context.getLangOpts().SemanticInterposition && + (Context.getLangOpts().PICLevel && !Context.getLangOpts().PIE)) +// Require various optimization to respect semantic interposition.

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62209 tests passed, 0 failed and 815 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille marked 2 inline comments as done. serge-sans-paille added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2685 + if (Args.hasArg(OPT_fsemantic_interposition)) +Opts.SemanticInterposition = 1; + MaskRay wrote: > `

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 240486. serge-sans-paille added a comment. Improve & comment examples Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/ https://reviews.llvm.org/D72829 Files:

[PATCH] D72829: Implement -fsemantic-interposition

MaskRay added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2685 + if (Args.hasArg(OPT_fsemantic_interposition)) +Opts.SemanticInterposition = 1; + ` Opts.SemanticInterposition = Args.hasArg(OPT_fsemantic_interposition);`

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon times-circle color=red} Unit tests: fail. 62174 tests passed, 5 failed and 813 were skipped. failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 240187. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/ https://reviews.llvm.org/D72829 Files: clang/docs/ClangCommandLineReference.rst clang/include/clang/Basic/LangOptions.def

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon times-circle color=red} Unit tests: fail. 62155 tests passed, 5 failed and 811 were skipped. failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 240110. serge-sans-paille added a comment. Formatting nits + rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/ https://reviews.llvm.org/D72829 Files:

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon times-circle color=red} Unit tests: fail. 62147 tests passed, 5 failed and 811 were skipped. failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp

[PATCH] D72829: Implement -fsemantic-interposition

MaskRay accepted this revision. MaskRay added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:488 + if (Context.getLangOpts().SemanticInterposition && + (Context.getLangOpts().PICLevel && !Context.getLangOpts().PIE)) +// Require various optimization to

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 239996. serge-sans-paille added a comment. Take into account pie/pic interaction with semantic-interposition More test case Remove premature release note entry Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille added a comment. > I feel that we are still behind a complete `-fsemantic-interposition`... I > don't know whether we should mention that the option is experimental. So do I. I can mention that in the -help output and in the changelog. Repository: rG LLVM Github Monorepo

[PATCH] D72829: Implement -fsemantic-interposition

MaskRay added a comment. In D72829#1835933 , @serge-sans-paille wrote: > Add Release note and doc. @MaskRay can you confirm current state is ok? I feel that we are still behind a complete `-fsemantic-interposition`... I don't know whether we should

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62135 tests passed, 0 failed and 808 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 239857. serge-sans-paille added a comment. Add Release note and doc. @MaskRay can you confirm current state is ok? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62132 tests passed, 0 failed and 808 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 239837. serge-sans-paille added a comment. Formatting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/ https://reviews.llvm.org/D72829 Files: clang/include/clang/Basic/LangOptions.def

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62134 tests passed, 0 failed and 808 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 239809. serge-sans-paille added a comment. Take review into account Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/ https://reviews.llvm.org/D72829 Files:

[PATCH] D72829: Implement -fsemantic-interposition

grimar added inline comments. Comment at: llvm/lib/IR/Module.cpp:558 +bool Module::getSemanticInterposition() const { + auto *Val = + cast_or_null(getModuleFlag("SemanticInterposition")); A minor nit about style: This probably would be nicer as ``` if

[PATCH] D72829: Implement -fsemantic-interposition

MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: llvm/include/llvm/IR/GlobalValue.h:287 + bool isDSOPreemptable() const { return !IsDSOLocal; } + MaskRay wrote: > It seems that this

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62107 tests passed, 0 failed and 808 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille added a comment. > I tried modifying `GlobalValue::maybeSetDsoLocal`: `setDSOLocal(true)` for > ExternalLinkage. Unfortunately that will break 1337 tests. If not so many > tests need fixing, I wish we can place `getParent() && > getParent()->getSemanticInterposition()` logic

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 239598. serge-sans-paille added a comment. - extra test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/ https://reviews.llvm.org/D72829 Files:

[PATCH] D72829: Implement -fsemantic-interposition

MaskRay added a comment. In D72829#1831017 , @serge-sans-paille wrote: > > Linkages which were not interposable before and can be interposable now: > > available_externally, linkonce_odr, weak_odr, external, and appending. > > @MaskRay I understand the

[PATCH] D72829: Implement -fsemantic-interposition

MaskRay added inline comments. Comment at: llvm/lib/IR/Globals.cpp:101 +return true; + return isInterposableLinkage(getLinkage()); +} MaskRay wrote: > MaskRay wrote: > > Checking `isInterposableLinkage(getLinkage())` first may be more efficient. > `if

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62055 tests passed, 0 failed and 784 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille added a comment. > Linkages which were not interposable before and can be interposable now: > available_externally, linkonce_odr, weak_odr, external, and appending. @MaskRay I understand the motivation behind that sentence, but do we want that change to be non-conditional,

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille updated this revision to Diff 239286. serge-sans-paille added a comment. Take into account review Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/ https://reviews.llvm.org/D72829 Files:

[PATCH] D72829: Implement -fsemantic-interposition

MaskRay added inline comments. Comment at: llvm/lib/IR/Globals.cpp:101 +return true; + return isInterposableLinkage(getLinkage()); +} MaskRay wrote: > Checking `isInterposableLinkage(getLinkage())` first may be more efficient. `if

[PATCH] D72829: Implement -fsemantic-interposition

MaskRay added a comment. The logic looks good to me. Linkages which were not interposable before and can be interposable now: available_externally, linkonce_odr, weak_odr, external, and appending. `isDefinitionExact` returned true for external before, and can return false if

[PATCH] D72829: Implement -fsemantic-interposition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61912 tests passed, 0 failed and 783 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille added a comment. @MaskRay no example yet, I'd like your opinion on the approach first. It has the advantage of being non intrusive to the code base... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72829/new/

[PATCH] D72829: Implement -fsemantic-interposition

serge-sans-paille created this revision. serge-sans-paille added reviewers: MaskRay, hfinkel. Herald added subscribers: llvm-commits, cfe-commits, haicheng, hiraditya, eraman. Herald added projects: clang, LLVM. serge-sans-paille added a comment. @MaskRay no example yet, I'd like your opinion on