-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[ubsan] Change "Type mismatch in operation" trap reason to "Alignment, null, or object-size error" #169752
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
Conversation
…, null and/or object-size error" I originally proposed this rewording when trap reasons were introduced in llvm#145967 (comment). At the time, there was a counter-proposal to split the enum; however, that work appears to have stalled (llvm#151243). In the meantime, there has been an additional datapoint that the current wording is confusing to users. Thus, let's reword it now to prevent further confusion.
|
@llvm/pr-subscribers-clang Author: Thurston Dang (thurstond) ChangesI originally proposed this rewording when trap reasons were introduced in #145967 (comment). At the time, there was a counter-proposal to split the enum; however, that work appears to have stalled (#151243). In the meantime, there has been an additional datapoint that the current wording is confusing to users. Thus, let's reword it now to prevent further confusion. Full diff: https://github.com/llvm/llvm-project/pull/169752.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/SanitizerHandler.h b/clang/lib/CodeGen/SanitizerHandler.h
index a66e7ab354eb2..ff932a9c452e1 100644
--- a/clang/lib/CodeGen/SanitizerHandler.h
+++ b/clang/lib/CodeGen/SanitizerHandler.h
@@ -64,7 +64,7 @@
SANITIZER_CHECK(SubOverflow, sub_overflow, 0, \
"Integer subtraction overflowed") \
SANITIZER_CHECK(TypeMismatch, type_mismatch, 1, \
- "Type mismatch in operation") \
+ "Alignment, null and/or object-size error") \
SANITIZER_CHECK(AlignmentAssumption, alignment_assumption, 0, \
"Alignment assumption violated") \
SANITIZER_CHECK( \
diff --git a/clang/test/DebugInfo/Generic/ubsan-trap-reason-type-mismatch.c b/clang/test/DebugInfo/Generic/ubsan-trap-reason-type-mismatch.c
index 802ec91b53a0d..a4c4c8cd8db2f 100644
--- a/clang/test/DebugInfo/Generic/ubsan-trap-reason-type-mismatch.c
+++ b/clang/test/DebugInfo/Generic/ubsan-trap-reason-type-mismatch.c
@@ -6,4 +6,4 @@ int type_mismatch(int *p) { return *p; }
// CHECK-LABEL: @type_mismatch
// CHECK: call void @llvm.ubsantrap(i8 22) {{.*}}!dbg [[LOC:![0-9]+]]
// CHECK: [[LOC]] = !DILocation(line: 0, scope: [[MSG:![0-9]+]], {{.+}})
-// CHECK: [[MSG]] = distinct !DISubprogram(name: "__clang_trap_msg$Undefined Behavior Sanitizer$Type mismatch in operation"
+// CHECK: [[MSG]] = distinct !DISubprogram(name: "__clang_trap_msg$Undefined Behavior Sanitizer$Alignment, null and/or object-size error"
|
|
@llvm/pr-subscribers-clang-codegen Author: Thurston Dang (thurstond) ChangesI originally proposed this rewording when trap reasons were introduced in #145967 (comment). At the time, there was a counter-proposal to split the enum; however, that work appears to have stalled (#151243). In the meantime, there has been an additional datapoint that the current wording is confusing to users. Thus, let's reword it now to prevent further confusion. Full diff: https://github.com/llvm/llvm-project/pull/169752.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/SanitizerHandler.h b/clang/lib/CodeGen/SanitizerHandler.h
index a66e7ab354eb2..ff932a9c452e1 100644
--- a/clang/lib/CodeGen/SanitizerHandler.h
+++ b/clang/lib/CodeGen/SanitizerHandler.h
@@ -64,7 +64,7 @@
SANITIZER_CHECK(SubOverflow, sub_overflow, 0, \
"Integer subtraction overflowed") \
SANITIZER_CHECK(TypeMismatch, type_mismatch, 1, \
- "Type mismatch in operation") \
+ "Alignment, null and/or object-size error") \
SANITIZER_CHECK(AlignmentAssumption, alignment_assumption, 0, \
"Alignment assumption violated") \
SANITIZER_CHECK( \
diff --git a/clang/test/DebugInfo/Generic/ubsan-trap-reason-type-mismatch.c b/clang/test/DebugInfo/Generic/ubsan-trap-reason-type-mismatch.c
index 802ec91b53a0d..a4c4c8cd8db2f 100644
--- a/clang/test/DebugInfo/Generic/ubsan-trap-reason-type-mismatch.c
+++ b/clang/test/DebugInfo/Generic/ubsan-trap-reason-type-mismatch.c
@@ -6,4 +6,4 @@ int type_mismatch(int *p) { return *p; }
// CHECK-LABEL: @type_mismatch
// CHECK: call void @llvm.ubsantrap(i8 22) {{.*}}!dbg [[LOC:![0-9]+]]
// CHECK: [[LOC]] = !DILocation(line: 0, scope: [[MSG:![0-9]+]], {{.+}})
-// CHECK: [[MSG]] = distinct !DISubprogram(name: "__clang_trap_msg$Undefined Behavior Sanitizer$Type mismatch in operation"
+// CHECK: [[MSG]] = distinct !DISubprogram(name: "__clang_trap_msg$Undefined Behavior Sanitizer$Alignment, null and/or object-size error"
|
|
@anthonyhatran @alazarev FYI (GitHub won't let me add you as reviewers) |
May I suggest "Alignment, null, or object-size error" to make it clearer that this could be an error related to any of the three. |
vitalybuka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enum is not split because we realized that it increases the binary size. These checks are most expensive, and it will make the impact even worse.
Maybe it's OK to split in "no-merge" mode.
Please wait for @delcypher
Thanks for the suggestion, changed. |
Ack |
|
LGTM. Thanks. |
I originally proposed this rewording when trap reasons were introduced in #145967 (comment). This was not adopted because there was a counter-proposal to split the enum; however, that work appears to have stalled (#151243). In the meantime, there has been an additional datapoint that the current wording is confusing to users. Thus, let's reword it now to prevent further confusion.