Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for builtin_verbose_trap #79230

Merged
merged 19 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions clang/docs/LanguageExtensions.rst
Original file line number Diff line number Diff line change
Expand Up @@ -3467,6 +3467,60 @@ Query for this feature with ``__has_builtin(__builtin_trap)``.

``__builtin_arm_trap`` is lowered to the ``llvm.aarch64.break`` builtin, and then to ``brk #payload``.

``__builtin_verbose_trap``
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahatanak Is this in the wrong place in the document? __builtin_verbose_trap is lexicographically after __builtin_nondeterministic_value

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like the existing builtins are in lexicographical order. __builtin_debugtrap is followed by __builtin_trap, which is followed by __builtin_nondeterministic_value.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK - that seems like more reason the prefix should be singular. Like we shouldn't add a new/distinct prefix for array bounds? Or other things we want to error out on - because it'd be an ongoing maintenance problem updating consumers like lldb each time we add one?

So maybe we should have a generic prefix we'll always use (__builtin_trap, say) and then maybe some identifier after that in a known format (eg: space, identifier, space) that can be used? then an error message string?

--------------------------

``__builtin_verbose_trap`` causes the program to stop its execution abnormally
and shows a human-readable description of the reason for the termination when a
debugger is attached or in a symbolicated crash log.

**Syntax**:

.. code-block:: c++

__builtin_verbose_trap(const char *category, const char *reason)

**Description**

``__builtin_verbose_trap`` is lowered to the ` ``llvm.trap`` <https://llvm.org/docs/LangRef.html#llvm-trap-intrinsic>`_ builtin.
Additionally, clang emits debugging information that represents an artificial
inline frame whose name encodes the category and reason strings passed to the builtin,
prefixed by a "magic" prefix.

For example, consider the following code:

.. code-block:: c++

void foo(int* p) {
if (p == nullptr)
__builtin_verbose_trap("check null", "Argument must not be null!");
}

The debugging information would look as if it were produced for the following code:

.. code-block:: c++

__attribute__((always_inline))
inline void "__clang_trap_msg$check null$Argument must not be null!"() {
__builtin_trap();
}

void foo(int* p) {
if (p == nullptr)
"__clang_trap_msg$check null$Argument must not be null!"();
}

However, the generated code would not actually contain a call to the artificial
function — it only exists in the debugging information.

Query for this feature with ``__has_builtin(__builtin_verbose_trap)``. Note that
users need to enable debug information to enable this feature. A call to this
builtin is equivalent to a call to ``__builtin_trap`` if debug information isn't
enabled.

The optimizer can merge calls to trap with different messages, which degrades
the debugging experience.

``__builtin_allow_runtime_check``
---------------------------------

Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/AST/Expr.h
Original file line number Diff line number Diff line change
Expand Up @@ -787,6 +787,11 @@ class Expr : public ValueStmt {
const Expr *PtrExpression, ASTContext &Ctx,
EvalResult &Status) const;

/// If the current Expr can be evaluated to a pointer to a null-terminated
/// constant string, return the constant string (without the terminating
/// null).
std::optional<std::string> tryEvaluateString(ASTContext &Ctx) const;

/// Enumeration used to describe the kind of Null pointer constant
/// returned from \c isNullPointerConstant().
enum NullPointerConstantKind {
Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/Basic/Builtins.td
Original file line number Diff line number Diff line change
Expand Up @@ -1152,6 +1152,12 @@ def Trap : Builtin {
let Prototype = "void()";
}

def VerboseTrap : Builtin {
let Spellings = ["__builtin_verbose_trap"];
let Attributes = [NoThrow, NoReturn];
let Prototype = "void(char const*, char const*)";
}

def Debugtrap : Builtin {
let Spellings = ["__builtin_debugtrap"];
let Attributes = [NoThrow];
Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -8875,6 +8875,8 @@ def err_expected_callable_argument : Error<
"expected a callable expression as %ordinal0 argument to %1, found %2">;
def note_building_builtin_dump_struct_call : Note<
"in call to printing function with arguments '(%0)' while dumping struct">;
def err_builtin_verbose_trap_arg : Error<
"argument to __builtin_verbose_trap must %select{be a pointer to a constant string|not contain $}0">;

def err_atomic_load_store_uses_lib : Error<
"atomic %select{load|store}0 requires runtime support that is not "
Expand Down
4 changes: 4 additions & 0 deletions clang/include/clang/CodeGen/ModuleBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

#include "clang/AST/ASTConsumer.h"
#include "clang/Basic/LLVM.h"
#include "llvm/ADT/StringRef.h"

namespace llvm {
class Constant;
Expand All @@ -27,6 +28,9 @@ namespace llvm {
}
}

// Prefix of the name of the artificial inline frame.
inline constexpr llvm::StringRef ClangTrapPrefix = "__clang_trap_msg";

namespace clang {
class CodeGenOptions;
class CoverageSourceInfo;
Expand Down
21 changes: 18 additions & 3 deletions clang/lib/AST/ExprConstant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1884,7 +1884,8 @@ static bool EvaluateAtomic(const Expr *E, const LValue *This, APValue &Result,
EvalInfo &Info);
static bool EvaluateAsRValue(EvalInfo &Info, const Expr *E, APValue &Result);
static bool EvaluateBuiltinStrLen(const Expr *E, uint64_t &Result,
EvalInfo &Info);
EvalInfo &Info,
std::string *StringResult = nullptr);

/// Evaluate an integer or fixed point expression into an APResult.
static bool EvaluateFixedPointOrInteger(const Expr *E, APFixedPoint &Result,
Expand Down Expand Up @@ -16792,7 +16793,7 @@ bool Expr::tryEvaluateObjectSize(uint64_t &Result, ASTContext &Ctx,
}

static bool EvaluateBuiltinStrLen(const Expr *E, uint64_t &Result,
EvalInfo &Info) {
EvalInfo &Info, std::string *StringResult) {
if (!E->getType()->hasPointerRepresentation() || !E->isPRValue())
return false;

Expand All @@ -16819,6 +16820,8 @@ static bool EvaluateBuiltinStrLen(const Expr *E, uint64_t &Result,
Str = Str.substr(0, Pos);

Result = Str.size();
if (StringResult)
*StringResult = Str;
return true;
}

Expand All @@ -16834,12 +16837,24 @@ static bool EvaluateBuiltinStrLen(const Expr *E, uint64_t &Result,
if (!Char.getInt()) {
Result = Strlen;
return true;
}
} else if (StringResult)
StringResult->push_back(Char.getInt().getExtValue());
if (!HandleLValueArrayAdjustment(Info, E, String, CharTy, 1))
return false;
}
}

std::optional<std::string> Expr::tryEvaluateString(ASTContext &Ctx) const {
Expr::EvalStatus Status;
EvalInfo Info(Ctx, Status, EvalInfo::EM_ConstantFold);
uint64_t Result;
std::string StringResult;

if (EvaluateBuiltinStrLen(this, Result, Info, &StringResult))
return StringResult;
return {};
}

bool Expr::EvaluateCharRangeAsString(std::string &Result,
const Expr *SizeExpression,
const Expr *PtrExpression, ASTContext &Ctx,
Expand Down
12 changes: 12 additions & 0 deletions clang/lib/CodeGen/CGBuiltin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3586,6 +3586,18 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
case Builtin::BI__builtin_trap:
EmitTrapCall(Intrinsic::trap);
return RValue::get(nullptr);
case Builtin::BI__builtin_verbose_trap: {
llvm::DILocation *TrapLocation = Builder.getCurrentDebugLocation();
if (getDebugInfo()) {
TrapLocation = getDebugInfo()->CreateTrapFailureMessageFor(
TrapLocation, *E->getArg(0)->tryEvaluateString(getContext()),
*E->getArg(1)->tryEvaluateString(getContext()));
}
ApplyDebugLocation ApplyTrapDI(*this, TrapLocation);
// Currently no attempt is made to prevent traps from being merged.
EmitTrapCall(Intrinsic::trap);
delcypher marked this conversation as resolved.
Show resolved Hide resolved
return RValue::get(nullptr);
}
case Builtin::BI__debugbreak:
EmitTrapCall(Intrinsic::debugtrap);
return RValue::get(nullptr);
Expand Down
40 changes: 40 additions & 0 deletions clang/lib/CodeGen/CGDebugInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "clang/Basic/FileManager.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/Version.h"
#include "clang/CodeGen/ModuleBuilder.h"
#include "clang/Frontend/FrontendOptions.h"
#include "clang/Lex/HeaderSearchOptions.h"
#include "clang/Lex/ModuleMap.h"
Expand Down Expand Up @@ -1692,6 +1693,28 @@ llvm::DIType *CGDebugInfo::createFieldType(
offsetInBits, flags, debugType, Annotations);
}

llvm::DISubprogram *
CGDebugInfo::createInlinedTrapSubprogram(StringRef FuncName,
llvm::DIFile *FileScope) {
// We are caching the subprogram because we don't want to duplicate
// subprograms with the same message. Note that `SPFlagDefinition` prevents
// subprograms from being uniqued.
llvm::DISubprogram *&SP = InlinedTrapFuncMap[FuncName];
delcypher marked this conversation as resolved.
Show resolved Hide resolved

if (!SP) {
llvm::DISubroutineType *DIFnTy = DBuilder.createSubroutineType(nullptr);
SP = DBuilder.createFunction(
/*Scope=*/FileScope, /*Name=*/FuncName, /*LinkageName=*/StringRef(),
/*File=*/FileScope, /*LineNo=*/0, /*Ty=*/DIFnTy,
/*ScopeLine=*/0,
/*Flags=*/llvm::DINode::FlagArtificial,
/*SPFlags=*/llvm::DISubprogram::SPFlagDefinition,
/*TParams=*/nullptr, /*ThrownTypes=*/nullptr, /*Annotations=*/nullptr);
}

return SP;
}

void CGDebugInfo::CollectRecordLambdaFields(
const CXXRecordDecl *CXXDecl, SmallVectorImpl<llvm::Metadata *> &elements,
llvm::DIType *RecordTy) {
Expand Down Expand Up @@ -3488,6 +3511,23 @@ llvm::DIMacroFile *CGDebugInfo::CreateTempMacroFile(llvm::DIMacroFile *Parent,
return DBuilder.createTempMacroFile(Parent, Line, FName);
}

llvm::DILocation *CGDebugInfo::CreateTrapFailureMessageFor(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's currently only one caller with a fixed parameter for Prefix - perhaps that could be hardcoded in the implementation instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dwblaikie We currently use this function for -fbounds-safety internally. We are currently in the process of upstreaming this which is why the function should not hardcode the Prefix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dwblaikie what do you think? -fbounds-safety is currently being upstreamed and there is going to be another caller of the function in the not-too-distant future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dwblaikie any comments?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with https://github.com/llvm/llvm-project/pull/79230/files#r1529429173 then - the prefix should be a parameter and I don't think it needs to be a #defined constant (CLANG_VERBOSE_TRAP_PREFIX) - and can instead be written as a literal in the caller in CGBuiltin.cpp?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll make it a parameter then.

I added macro CLANG_VERBOSE_TRAP_PREFIX to ModuleBuilder.h so that lldb wouldn't have to define it too. See #80368 (comment).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dwblaikie 's comment:
#79230 (comment)

So the name of the subprogram would be something like __builtin_trap: identifier123 : Argument_must_not_be_null?

@Michael137 @delcypher is there a reason we cannot use the same prefix (__builtin_trap, for example)? Does lldb need clang to use different prefixes for -fbounds-safety and builtin_verbose_trap?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the patch to pass a category string to the builtin. The artificial function name has the format <prefix>:<category>:<message> where <prefix> is always __builtin_verbose_trap.

We should probably use something other than : for the separator as users might want to use it in the category or message string (e.g., boost::some_library_name).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dwblaikie any other comments?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, I'm not too fussed about the separator for the prefix (we control that) and category (will the category be user-visible? Like a warning group? Or is that only still an implementation detail/contract between DWARF producer and DWARF consumer? I thought it was more the latter/implementation detail? In which case we can say what characters are valid there, and I'd keep it pretty simple, like the warning group names - all lower, dash or underscore separated seems fine)

llvm::DebugLoc TrapLocation, StringRef Category, StringRef FailureMsg) {
// Create a debug location from `TrapLocation` that adds an artificial inline
// frame.
SmallString<64> FuncName(ClangTrapPrefix);

FuncName += "$";
FuncName += Category;
FuncName += "$";
FuncName += FailureMsg;

llvm::DISubprogram *TrapSP =
createInlinedTrapSubprogram(FuncName, TrapLocation->getFile());
return llvm::DILocation::get(CGM.getLLVMContext(), /*Line=*/0, /*Column=*/0,
/*Scope=*/TrapSP, /*InlinedAt=*/TrapLocation);
}

static QualType UnwrapTypeForDebugInfo(QualType T, const ASTContext &C) {
Qualifiers Quals;
do {
Expand Down
22 changes: 22 additions & 0 deletions clang/lib/CodeGen/CGDebugInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@
#include "llvm/IR/DebugInfo.h"
#include "llvm/IR/ValueHandle.h"
#include "llvm/Support/Allocator.h"
#include <map>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to not belong to this commit?

#include <optional>
#include <string>

namespace llvm {
class MDNode;
Expand Down Expand Up @@ -346,6 +348,14 @@ class CGDebugInfo {
const FieldDecl *BitFieldDecl, const llvm::DIDerivedType *BitFieldDI,
llvm::ArrayRef<llvm::Metadata *> PreviousFieldsDI, const RecordDecl *RD);

/// A cache that maps names of artificial inlined functions to subprograms.
llvm::StringMap<llvm::DISubprogram *> InlinedTrapFuncMap;
delcypher marked this conversation as resolved.
Show resolved Hide resolved

/// A function that returns the subprogram corresponding to the artificial
/// inlined function for traps.
llvm::DISubprogram *createInlinedTrapSubprogram(StringRef FuncName,
llvm::DIFile *FileScope);

/// Helpers for collecting fields of a record.
/// @{
void CollectRecordLambdaFields(const CXXRecordDecl *CXXDecl,
Expand Down Expand Up @@ -602,6 +612,18 @@ class CGDebugInfo {
return CoroutineParameterMappings;
}

/// Create a debug location from `TrapLocation` that adds an artificial inline
/// frame where the frame name is
///
/// * `<Prefix>:<Category>:<FailureMsg>`
///
/// `<Prefix>` is "__clang_trap_msg".
///
/// This is used to store failure reasons for traps.
llvm::DILocation *CreateTrapFailureMessageFor(llvm::DebugLoc TrapLocation,
StringRef Category,
StringRef FailureMsg);

private:
/// Emit call to llvm.dbg.declare for a variable declaration.
/// Returns a pointer to the DILocalVariable associated with the
Expand Down
32 changes: 32 additions & 0 deletions clang/lib/Sema/SemaChecking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,33 @@ static bool checkArgCount(Sema &S, CallExpr *Call, unsigned DesiredArgCount) {
<< /*is non object*/ 0 << Call->getArg(1)->getSourceRange();
}

static bool checkBuiltinVerboseTrap(CallExpr *Call, Sema &S) {
ahatanak marked this conversation as resolved.
Show resolved Hide resolved
bool HasError = false;

for (unsigned I = 0; I < Call->getNumArgs(); ++I) {
Expr *Arg = Call->getArg(I);

if (Arg->isValueDependent())
continue;

std::optional<std::string> ArgString = Arg->tryEvaluateString(S.Context);
int DiagMsgKind = -1;
// Arguments must be pointers to constant strings and cannot use '$'.
if (!ArgString.has_value())
DiagMsgKind = 0;
else if (ArgString->find('$') != std::string::npos)
DiagMsgKind = 1;

if (DiagMsgKind >= 0) {
S.Diag(Arg->getBeginLoc(), diag::err_builtin_verbose_trap_arg)
<< DiagMsgKind << Arg->getSourceRange();
HasError = true;
}
}

return !HasError;
}

static bool convertArgumentToType(Sema &S, Expr *&Value, QualType Ty) {
if (Value->isTypeDependent())
return false;
Expand Down Expand Up @@ -3204,6 +3231,11 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
case Builtin::BI__builtin_matrix_column_major_store:
return BuiltinMatrixColumnMajorStore(TheCall, TheCallResult);

case Builtin::BI__builtin_verbose_trap:
if (!checkBuiltinVerboseTrap(TheCall, *this))
return ExprError();
break;

case Builtin::BI__builtin_get_device_side_mangled_name: {
auto Check = [](CallExpr *TheCall) {
if (TheCall->getNumArgs() != 1)
Expand Down
54 changes: 54 additions & 0 deletions clang/test/CodeGenCXX/debug-info-verbose-trap.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// RUN: %clang_cc1 -triple arm64-apple-ios -std=c++20 -emit-llvm -debug-info-kind=limited %s -o - | FileCheck %s
delcypher marked this conversation as resolved.
Show resolved Hide resolved

// CHECK-LABEL: define void @_Z2f0v()
// CHECK: call void @llvm.trap(), !dbg ![[LOC17:.*]]

// CHECK: declare void @llvm.trap() #[[ATTR1:.*]]

// CHECK-LABEL: define void @_Z2f1v()
// CHECK: call void @llvm.trap(), !dbg ![[LOC23:.*]]
// CHECK: call void @llvm.trap(), !dbg ![[LOC25:.*]]

// CHECK-LABEL: define void @_Z2f3v()
// CHECK: call void @_Z2f2IXadsoKcL_ZL8constCatEEEXadsoS0_L_ZL8constMsgEEEEvv()

// CHECK-LABEL: define internal void @_Z2f2IXadsoKcL_ZL8constCatEEEXadsoS0_L_ZL8constMsgEEEEvv
// CHECK: call void @llvm.trap(), !dbg ![[LOC36:.*]]

// CHECK: attributes #[[ATTR1]] = { cold {{.*}}}

// CHECK: ![[FILESCOPE:.*]] = !DIFile(filename: "{{.*}}debug-info-verbose-trap.cpp"

delcypher marked this conversation as resolved.
Show resolved Hide resolved
char const constCat[] = "category2";
char const constMsg[] = "hello";

// CHECK: ![[SUBPROG14:.*]] = distinct !DISubprogram(name: "f0", linkageName: "_Z2f0v",
// CHECK: ![[LOC17]] = !DILocation(line: 0, scope: ![[SUBPROG18:.*]], inlinedAt: ![[LOC20:.*]])
// CHECK: ![[SUBPROG18]] = distinct !DISubprogram(name: "__clang_trap_msg$category1$Argument_must_not_be_null", scope: ![[FILESCOPE]], file: ![[FILESCOPE]], type: !{{.*}}, flags: DIFlagArtificial, spFlags: DISPFlagDefinition, unit: !{{.*}})
// CHECK: ![[LOC20]] = !DILocation(line: [[@LINE+2]], column: 3, scope: ![[SUBPROG14]])
void f0() {
__builtin_verbose_trap("category1", "Argument_must_not_be_null");
}

// CHECK: ![[SUBPROG22:.*]] = distinct !DISubprogram(name: "f1", linkageName: "_Z2f1v",
// CHECK: ![[LOC23]] = !DILocation(line: 0, scope: ![[SUBPROG18]], inlinedAt: ![[LOC24:.*]])
// CHECK: ![[LOC24]] = !DILocation(line: [[@LINE+5]], column: 3, scope: ![[SUBPROG22]])
// CHECK: ![[LOC25]] = !DILocation(line: 0, scope: ![[SUBPROG26:.*]], inlinedAt: ![[LOC27:.*]])
// CHECK: ![[SUBPROG26]] = distinct !DISubprogram(name: "__clang_trap_msg$category2$hello", scope: ![[FILESCOPE]], file: ![[FILESCOPE]], type: !{{.*}}, flags: DIFlagArtificial, spFlags: DISPFlagDefinition, unit: !{{.*}})
// CHECK: ![[LOC27]] = !DILocation(line: [[@LINE+3]], column: 3, scope: ![[SUBPROG22]])
void f1() {
__builtin_verbose_trap("category1", "Argument_must_not_be_null");
__builtin_verbose_trap("category2", "hello");
}

// CHECK: ![[SUBPROG32:.*]] = distinct !DISubprogram(name: "f2<constCat, constMsg>", linkageName: "_Z2f2IXadsoKcL_ZL8constCatEEEXadsoS0_L_ZL8constMsgEEEEvv",
// CHECK: ![[LOC36]] = !DILocation(line: 0, scope: ![[SUBPROG26]], inlinedAt: ![[LOC37:.*]])
// CHECK: ![[LOC37]] = !DILocation(line: [[@LINE+3]], column: 3, scope: ![[SUBPROG32]])
template <const char * const category, const char * const reason>
void f2() {
__builtin_verbose_trap(category, reason);
}

void f3() {
f2<constCat, constMsg>();
}
Loading