Page MenuHomePhabricator

Implement -fsemantic-interposition
ClosedPublic

Authored by serge-sans-paille on Jan 16 2020, 3:07 AM.

Details

Summary

First attempt at implementing -fsemantic-interposition.

Rely on GlobalValue::isInterposable that already captures most of the expected behavior.

Rely on a ModuleFlag to state whether we should respect SemanticInterposition or not. The default remains no.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
MaskRay added inline comments.Tue, Jan 21, 11:56 AM
llvm/lib/IR/Module.cpp
565

A test/llvm/Verifier/ test checking "SemanticInterposition" must be a ConstantInt will be nice.

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, i.e. not guarded by -fsemantic-interposition ?

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 in maybeSetDsoLocal and simplify isInterposable.

if (!isInterposableLinkage(getLinkage()))
  return false;
- return getParent() && getParent()->getSemanticInterposition() && !isDSOLocal();
+ return !isDSOLocal();
  • extra test case

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 in maybeSetDsoLocal and simplify isInterposable.

Tried that and ended up with various check failures.
I also tried to update isInterposableLinkage to either

  • set more linkage type as interposable unconditionnaly, and with no surprise, it triggers plenty check failures
  • accept a mandatory ยซ HonorSemanticInterposition argument, so that both isInterposableLinkage and isInterposable are consistent, but in some cases, isInterposableLinkage doesn't have a module reference to gather the ModuleFlag from...

Unit tests: pass. 62107 tests passed, 0 failed and 808 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

MaskRay accepted this revision.Wed, Jan 22, 10:04 AM
MaskRay added inline comments.
llvm/include/llvm/IR/GlobalValue.h
287

isDSOPreemptable is not used in the new patch. Forgot to remove?

llvm/include/llvm/IR/Module.h
851

It does not seem that we use @{ @} nowadays... They don't add much documentation value so they can probably be removed.

This revision is now accepted and ready to land.Wed, Jan 22, 10:04 AM
grimar added a subscriber: grimar.Thu, Jan 23, 1:04 AM
grimar added inline comments.
llvm/lib/IR/Module.cpp
559

A minor nit about style:
This probably would be nicer as

if (auto *Val = cast_or_null<ConstantAsMetadata>(
        getModuleFlag("SemanticInterposition")))
  return cast<ConstantInt>(Val->getValue())->getZExtValue();
return false;

Take review into account

Unit tests: pass. 62134 tests passed, 0 failed and 808 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

serge-sans-paille marked 7 inline comments as done.Thu, Jan 23, 2:57 AM

Unit tests: pass. 62132 tests passed, 0 failed and 808 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Add Release note and doc. @MaskRay can you confirm current state is ok?

Unit tests: pass. 62135 tests passed, 0 failed and 808 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please | join beta or | enable it for your project.

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 mention that the option is experimental.

clang/lib/CodeGen/CodeGenModule.cpp
487

This should also check -fno-pic -fpie vs -fpic.

In GCC, only -fpic -fPIC enable semantic interposition.

-fsemantic-interpositions seems a no-op with -fno-pic or -fpie.

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.

Take into account pie/pic interaction with semantic-interposition
More test case
Remove premature release note entry

MaskRay accepted this revision.Thu, Jan 23, 1:48 PM
MaskRay added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
488

The parentheses can be deleted.

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
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Formatting nits + rebase

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
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

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
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

MaskRay added inline comments.Fri, Jan 24, 9:17 AM
clang/lib/Frontend/CompilerInvocation.cpp
2715

Opts.SemanticInterposition = Args.hasArg(OPT_fsemantic_interposition);

clang/test/CodeGen/semantic-interposition.c
2

Add a file-level comment what configurations produce SemanticInterposition

9

CHECK-NOPIC-NOPIE-NOT and CHECK-PIC-PIE-NOT can be combined (e.g. INTERPOSE: vs NO-INTERPOSE-NOT: )

llvm/test/Transforms/Inline/inline-semantic-interposition.ll
2

excess space

Add a comment what happened here.

Improve & comment examples

serge-sans-paille marked 2 inline comments as done.Mon, Jan 27, 1:20 AM
serge-sans-paille added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
2715

That was on purpose, so that we're backward compatible with existing situation. Without the flag, the genrated .ll doesn't differ. That's less explicit though. Does it make sense to you?

llvm/test/Transforms/Inline/inline-semantic-interposition.ll
2

I'm not sure about the excess space comment, but comment added.

Unit tests: pass. 62209 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

MaskRay added inline comments.Mon, Jan 27, 9:46 AM
clang/lib/CodeGen/CodeGenModule.cpp
488

An alternative is to move the PIC/PIE logic to lib/Frontend/CompilerInvocation.cpp

Then we should be able to use if (Context.getLangOpts().SemanticInterposition) here

llvm/test/Transforms/Inline/inline-semantic-interposition.ll
2

Nit: there are two spaces between -inline and -S.

@MaskRay should we add a verifier step to check that pie/pic/semanticinterposition module flags are consistent, or leave that to clang ?

serge-sans-paille marked 3 inline comments as done.Tue, Jan 28, 2:18 AM
serge-sans-paille added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
488

๐Ÿ‘ like that one

Unit tests: pass. 62258 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

@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 "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, i.e. SemanticInterposition is in affect even if "PIE Level" is set.

MaskRay added inline comments.Tue, Jan 28, 10:32 AM
clang/lib/Frontend/CompilerInvocation.cpp
3041

Opts.SemanticInterposition is initialized to 0 and is not assigned elsewhere.

Opts.SemanticInterposition = ... should be possible.

serge-sans-paille marked an inline comment as done.
serge-sans-paille marked an inline comment as done.

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, i.e. SemanticInterposition is in affect even if "PIE Level" is set.

OK, looks good as is, then?

MaskRay accepted this revision.Tue, Jan 28, 12:28 PM

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, i.e. SemanticInterposition is in affect even if "PIE Level" is set.

OK, looks good as is, then?

Yes!

Unit tests: pass. 62281 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

arichardson added inline comments.
clang/docs/ClangCommandLineReference.rst
907

This looks like it should be reordered?

@arichardson doc updated, any other advice?

Unit tests: pass. 62286 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

@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 not clear to me what "Enable semantic interposition" means.
I had to read the code and look the GCC documentation+llvm mailing list posts to understand it.

As this is user-facing documentation I feel like there should be a slightly longer explaning what the option does.

For comparison here is the GCC command line flag documentation

-fsemantic-interposition
Some object formats, like ELF, allow interposing of symbols by the dynamic linker. This means that for symbols exported from the DSO, the compiler cannot perform interprocedural propagation, inlining and other optimizations in anticipation that the function or variable in question may change. While this feature is useful, for example, to rewrite memory allocation functions by a debugging implementation, it is expensive in the terms of code quality. With -fno-semantic-interposition the compiler assumes that if interposition happens for functions the overwriting function will have precisely the same semantics (and side effects). Similarly if interposition happens for variables, the constructor of the variable will be the same. The flag has no effect for functions explicitly declared inline (where it is never allowed for interposition to change semantics) and for symbols explicitly declared weak.

Other than this, the change looks good to me.

clang/docs/ClangCommandLineReference.rst
907

Sorry I mean that it looks like all other entries are .. option:: followed by the description but here it's the other way around. It seems like this change will make the documention of -fsanitize-* be Enable semantic interposition.

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.

Update user-level documentation.

As this is user-facing documentation I feel like there should be a slightly longer explaning what the option does.

done!

Unit tests: pass. 62300 tests passed, 0 failed and 837 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

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!

llvm/include/llvm/IR/GlobalValue.h
425โ€“426

Minor nit: We have extended this to now determine if a definition can be substituted with an arbitrary definition at link time or load time.

I would suggest changing the first link time to link time or load time, and changing
since the callee can be replaced with something arbitrary at link time. to
since the callee can be replaced with something arbitrary.,

take @sfertile review into account.

Unit tests: pass. 62312 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Thanks documentation looks much better now.

clang/docs/ClangCommandLineReference.rst
907

It still looks like the documentation here is for the wrong option. I think it should be:

.. option:: -fsanitize=<check>,<arg2>..., -fno-sanitize=<arg1>,<arg2>...

Turn on runtime checks for various forms of undefined or suspicious behavior. See user manual for available checks

.. option:: -fno-semantic-interposition, -fsemantic-interposition

Enable semantic interposition. Semantic interposition allows for the
interposition of a symbol by another at run time, thus preventing a range of
inter-procedural optimisation.

Fix documentation ordering, thank goes to @arichardson for spotting that.

Unit tests: pass. 62325 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

This revision was automatically updated to reflect the committed changes.