This is an archive of the discontinued LLVM Phabricator instance.

[llvm][IR] Add no_cfi constant
ClosedPublic

Authored by samitolvanen on Aug 20 2021, 11:58 AM.

Details

Summary

With Control-Flow Integrity (CFI), the LowerTypeTests pass replaces
function references with CFI jump table references, which is a problem
for low-level code that needs the address of the actual function body.

For example, in the Linux kernel, the code that sets up interrupt
handlers needs to take the address of the interrupt handler function
instead of the CFI jump table, as the jump table may not even be mapped
into memory when an interrupt is triggered.

This change adds the no_cfi constant type, which wraps function
references in a value that LowerTypeTestsModule::replaceCfiUses does not
replace.

Link: https://github.com/ClangBuiltLinux/linux/issues/1353

Diff Detail

Event Timeline

samitolvanen created this revision.Aug 20 2021, 11:58 AM
samitolvanen requested review of this revision.Aug 20 2021, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2021, 11:58 AM

Here's a PoC of the "no cfi" constant type suggested by Peter. PTAL.

I think you might want to update the syntax files for the various text editors. See:

  • llvm/utils/vim/syntax/llvm.vim
  • llvm/utils/emacs/llvm-mode.el
  • llvm/utils/kate/llvm.xml
  • llvm/utils/vscode/llvm/syntaxes/ll.tmLanguage.yaml
llvm/include/llvm/IR/Constants.h
936

plz fix linter warning

llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1778

Neat! I didn't know isa supported multiple types like this!

llvm/test/Bitcode/nocfivalue.ll
2–3

If the < is not necessary, let's drop it!

43

If @f1, @f2, @f4, and @f5 are "empty", want to simply use declare rather than define?

llvm/test/CodeGen/X86/nocfivalue.ll
2

Do we need <? If not, let's remove.

How does -asm-verbose=false change this test?

16–26

Can we change these to be simply declarations rather then empty function definitions?

llvm/test/Transforms/LowerTypeTests/nocfivalue.ll
10–23

declare?

samitolvanen marked 3 inline comments as done.

Fixed clang-tidy warnings, cleaned up tests.

I think you might want to update the syntax files for the various text editors. See:

  • llvm/utils/vim/syntax/llvm.vim
  • llvm/utils/emacs/llvm-mode.el
  • llvm/utils/kate/llvm.xml
  • llvm/utils/vscode/llvm/syntaxes/ll.tmLanguage.yaml

I was looking at what was done with dso_local_equivalent, which is similar constant type, and it looks like that's only in emacs/llvm-mode.el. I'm not sure how kate or vscode would display this keyword, but I added it to vim and emacs for now.

llvm/test/CodeGen/X86/nocfivalue.ll
2

How does -asm-verbose=false change this test?

It removes some comments from the output, which doesn't affect this test, but I suppose might reduce the probability of future breakages. I followed the example of other similar tests here.

16–26

Can we change these to be simply declarations rather then empty function definitions?

Only @f1 it seems. Using declare for an internal function results in an error, and the "cfi-canonical-jump-table" attribute is also ignored for a declaration.

llvm/test/Transforms/LowerTypeTests/nocfivalue.ll
10–23

declare?

Only @f1 again.

Fixed bitcast handling in NoCFIValue::handleOperandChangeImpl.

pcc added inline comments.Oct 27 2021, 5:16 PM
llvm/lib/IR/Constants.cpp
1993

Shouldn't this use stripPointerCasts instead? Otherwise you're potentially replacing this with a different global.

1997

Can this ever be true if we're asserting that this is a GlobalValue?

llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1777–1779

Prefer to say why, not what, in this comment.

Addressed pcc's comments.

samitolvanen marked 3 inline comments as done.Oct 29 2021, 3:23 PM
nickdesaulniers accepted this revision.Dec 6 2021, 11:30 AM

LGTM. @pcc do you have any additional feedback?

llvm/lib/IR/Constants.cpp
1993

I wouldn't use auto here. If we don't reuse To, then perhaps simply

To = To->stripPointerCasts();

would do.

1994–1997

or you could do:

auto *GV =  dyn_cast<GlobalValue>(To);
assert(GV && "Can only replace the operands with a global value");
This revision is now accepted and ready to land.Dec 6 2021, 11:30 AM
samitolvanen marked an inline comment as done.

Addressed Nick's comments about NoCFIValue::handleOperandChangeImpl.

pcc accepted this revision.Dec 15 2021, 3:25 PM

LGTM

This revision was automatically updated to reflect the committed changes.