[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

This revision was automatically updated to reflect the committed changes. Closed by commit rGec2e26eaf635: [Clang] Add __builtin_function_start (authored by samitolvanen). Changed prior to commit: https://reviews.llvm.org/D108479?vs=391159=395516#toc Repository: rG LLVM Github Monorepo

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

pcc accepted this revision. pcc added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/ https://reviews.llvm.org/D108479 ___

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall accepted this revision. rjmccall added a comment. This looks great, thanks. Please feel free to commit with the requested minor change to the docs. Comment at: clang/docs/LanguageExtensions.rst:2555 +This builtin is not supported on all targets. The returned

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 391159. samitolvanen marked 5 inline comments as done. samitolvanen added a comment. Addressed comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/ https://reviews.llvm.org/D108479 Files:

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 390521. samitolvanen added a comment. Use standard l-value conversions, and add a test case for `constexpr`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/ https://reviews.llvm.org/D108479

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen planned changes to this revision. samitolvanen added a comment. In D108479#3150298 , @rjmccall wrote: > Your builtin is using custom type-checking (`t`), which suppresses all the > normal conversions that happen on expressions.

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. Your builtin is using custom type-checking (`t`), which suppresses all the normal conversions that happen on expressions. Specifically, it skips lvalue-to-rvalue conversion, so in this example the argument ends up being an l-value reference to a variable rather than

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:208 +if (UnaryOp->getOpcode() == UnaryOperator::Opcode::UO_AddrOf) + E = UnaryOp->getSubExpr(); + rjmccall wrote: > samitolvanen wrote: > > rjmccall wrote: > > >

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 389346. samitolvanen added a comment. Changed the code to evaluate the argument as a constant expression. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/ https://reviews.llvm.org/D108479 Files:

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:208 +if (UnaryOp->getOpcode() == UnaryOperator::Opcode::UO_AddrOf) + E = UnaryOp->getSubExpr(); + samitolvanen wrote: > rjmccall wrote: > > samitolvanen wrote: > > > rjmccall

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen planned changes to this revision. samitolvanen added a comment. In D108479#3149297 , @rjmccall wrote: > In D108479#3149228 , @samitolvanen > wrote: > >> I worked around this for now by explicitly

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. In D108479#3149228 , @samitolvanen wrote: > I worked around this for now by explicitly allowing > `__builtin_function_start` in `CheckLValueConstantExpression`, but this seems > terribly hacky. What would be the correct way

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen added a comment. In D108479#3140688 , @rjmccall wrote: > In D108479#3140638 , @samitolvanen > wrote: > >> Sure, I can take a look at how that would work. Basically, in >>

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 389242. samitolvanen marked an inline comment as done. samitolvanen added a comment. Refactored to avoid evaluating the expression into an l-value. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen planned changes to this revision. samitolvanen added a comment. In D108479#3140688 , @rjmccall wrote: > In D108479#3140638 , @samitolvanen > wrote: > >> In D108479#3133578

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. In D108479#3140638 , @samitolvanen wrote: > In D108479#3133578 , @rjmccall > wrote: > >> If we do need to support constant expressions of this > > Yes, we need this also in constant

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen marked an inline comment as done. samitolvanen added a comment. In D108479#3133578 , @rjmccall wrote: > If we do need to support constant expressions of this Yes, we need this also in constant expressions. > I think we should have the

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 388237. samitolvanen added a comment. Renamed to `__builtin_function_start`, allowed only FunctionDecls as a parameter, added support for C++ member functions, and disallowed comparisons in integral constant expressions. Repository: rG LLVM Github

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. If we do need to support constant expressions of this, I think we should have the constant evaluator produce an expression rather than an l-value, the way we do with some other builtin calls. That should stop the comparison problem more generally. Repository: rG

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. Do any of the proposed use cases actually require this to be a constant expression? Some of these patterns can be cheaply implemented with code generation even if the target doesn't otherwise support constants; for example, we could just mask the THUMB bit off on

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

pcc added a comment. In D108479#3129577 , @rjmccall wrote: > In D108479#3129571 , @jrtc27 wrote: > >> For CHERI there's the added complication that descriptors and trampolines >> can exist for security reasons

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. In D108479#3129571 , @jrtc27 wrote: > For CHERI there's the added complication that descriptors and trampolines can > exist for security reasons when crossing security domains, and you absolutely > should not let one

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

jrtc27 added a comment. If it's bypassing the descriptors then __builtin_symbol_address is the wrong name (and a bit ambiguous). As far as dlsym is concerned, the symbol is the descriptor, but when you get down to the ELF representation itself that's not always true. For PPC64 ELFv1, the ELF

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. I don't know for certain, but I would guess that the kernel wants to get the address of the first instruction in the function for the purposes of some sort of later PC-based table lookup, which means that yes, it probably *does* need to bypass descriptors on CHERI /

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

jrtc27 added a comment. In D108479#3110003 , @rjmccall wrote: > `std::addressof()` certainly ought to return a signed pointer > under ptrauth, so if your goal here is to get a completely unadorned symbol > address, I think you do need a different

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. In D108479#3113055 , @pcc wrote: > (Adding back @rsmith, @rjmccall.) > > In D108479#3113035 , @samitolvanen > wrote: > >> In D108479#3112492 ,

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

pcc added subscribers: rjmccall, rsmith. pcc added a comment. (Adding back @rsmith, @rjmccall.) In D108479#3113035 , @samitolvanen wrote: > In D108479#3112492 , @rjmccall > wrote: > >> You could also make this

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen removed subscribers: rsmith, rjmccall. samitolvanen planned changes to this revision. samitolvanen added a comment. In D108479#3112492 , @rjmccall wrote: > You could also make this Just Work for things like C++ member functions > rather

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. Yeah, I think that's a better name. The documentation can say that ideally this also wouldn't include things like the THUMB bit, but there are technical limitations that make it hard to achieve that. You could also make this Just Work for things like C++ member

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

pcc added a comment. > Maybe it should even be semantically restricted to require a constant decl > reference as its operand? I think we should do this. Maybe it should be named something like `__builtin_symbol_address` if we're intending for this to have an effect with PAuth as well?

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 384897. samitolvanen added a comment. Keep a `void *` return type for the nocfi variant. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/ https://reviews.llvm.org/D108479 Files:

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. `std::addressof()` certainly ought to return a signed pointer under ptrauth, so if your goal here is to get a completely unadorned symbol address, I think you do need a different builtin, and it probably ought to return a `void*` to emphasize that it shouldn't be used

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

pcc requested changes to this revision. pcc added subscribers: rjmccall, rsmith. pcc added a comment. This revision now requires changes to proceed. In D108479#3108360 , @ardb wrote: > I would argue that the existing __builtin_addressof() should absorb

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

ardb added a comment. I would argue that the existing __builtin_addressof() should absorb this behavior, rather than adding a special builtin related to CFI. As is documented for __builtin_addressof(), its intended use is in cases where the & operator may return something other than the

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 383518. samitolvanen marked 2 inline comments as done. samitolvanen added a comment. Missed a comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/ https://reviews.llvm.org/D108479 Files:

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 383514. samitolvanen added a comment. Addressed comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/ https://reviews.llvm.org/D108479 Files: clang/docs/LanguageExtensions.rst

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

nickdesaulniers added inline comments. Comment at: clang/include/clang/AST/APValue.h:195 unsigned CallIndex, Version; + bool NoCFIValue : 1; }; Is there be padding off the end of the bitfield? Or does this actually change the

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

nickdesaulniers added inline comments. Comment at: clang/lib/AST/APValue.cpp:44-46 + false} {} APValue::LValueBase::LValueBase(const Expr *P, unsigned I, unsigned V) +: Ptr(P), Local{I, V, false} {}

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

nickdesaulniers added inline comments. Comment at: clang/test/CodeGen/builtin-addressof-nocfi.c:2 +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fsanitize=cfi-icall -o - %s | FileCheck %s + +void a(void) {} mind adding a test case to this file

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

pcc accepted this revision. pcc added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/AST/APValue.cpp:133 +void APValue::LValueBase::setNoCFIValue(bool NoCFI) { + if (is()) { +Local.NoCFIValue = NoCFI; Remove braces

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen added inline comments. Comment at: clang/test/CodeGen/builtin-addressof-nocfi.c:18 + // CHECK: call void @c(void ()* no_cfi @a) + c(__builtin_addressof_nocfi(a)); + e(); nickdesaulniers wrote: > do we ever need the builtin address of a global

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen updated this revision to Diff 369795. samitolvanen marked 2 inline comments as done. samitolvanen added a comment. Fixed clang-tidy warnings, dropped an unnecessary auto. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108479/new/

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

nickdesaulniers added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1890 -if (auto FD = dyn_cast(D)) - return CGM.GetAddrOfFunction(FD); +if (auto FD = dyn_cast(D)) { + auto *C = CGM.GetAddrOfFunction(FD); fix linter

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen added reviewers: nickdesaulniers, pcc. samitolvanen added a comment. Here's a PoC of the built-in function that returns the address of the function body with CFI. Based on D108478 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

samitolvanen created this revision. Herald added a subscriber: martong. samitolvanen requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This change adds __builtin_addressof_nocfi(). This built-in function is similar to the existing