[Clang][CodeGen] Use EmitLoadOfLValue instead of EmitLoadOfScalar to get LHS for complex compound assignment #166798

BStott6 · 2025-11-06T16:30:53Z

Fixes Code with complex and bool crashed compiler #166512
ComplexExprEmitter::EmitCompoundAssignLValue is calling EmitLoadOfScalar(LValue, SourceLocation) to load the LHS value in the case that it's non-complex, however this function requires that the value is a simple LValue - issue occurred because the LValue in question was a bitfield LValue. I changed it to use this function which seems to handle all of the different cases (deferring to the original EmitLoadOfScalar if it's a simple LValue)

Note that I am new to Clang and I'm not confident this is the right change; it makes sense to me, fixes the crash and passes all tests but please check carefully!

…get LHS for complex compound assignment

github-actions · 2025-11-06T16:31:16Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-11-06T16:31:54Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Benjamin Stott (BStott6)

Changes

Fixes Code with complex and bool crashed compiler #166512
ComplexExprEmitter::EmitCompoundAssignLValue is calling EmitLoadOfScalar(LValue, SourceLocation) to load the LHS value in the case that it's non-complex, however this function requires that the value is a simple LValue - issue occurred because the LValue in question was a bitfield LValue. I changed it to use this function which seems to handle all of the different cases (deferring to the original EmitLoadOfScalar if it's a simple LValue)

Note that I am new to Clang and I'm not confident this is the right change; it makes sense to me, fixes the crash and passes all tests but please check carefully!

Full diff: https://github.com/llvm/llvm-project/pull/166798.diff

1 Files Affected:

(modified) clang/lib/CodeGen/CGExprComplex.cpp (+1-1)

diff --git a/clang/lib/CodeGen/CGExprComplex.cpp b/clang/lib/CodeGen/CGExprComplex.cpp
index f8a946a76554a..47435758fcde7 100644
--- a/clang/lib/CodeGen/CGExprComplex.cpp
+++ b/clang/lib/CodeGen/CGExprComplex.cpp
@@ -1283,7 +1283,7 @@ EmitCompoundAssignLValue(const CompoundAssignOperator *E,
     else
       OpInfo.LHS = EmitComplexToComplexCast(LHSVal, LHSTy, OpInfo.Ty, Loc);
   } else {
-    llvm::Value *LHSVal = CGF.EmitLoadOfScalar(LHS, Loc);
+    llvm::Value *LHSVal = CGF.EmitLoadOfLValue(LHS, Loc).getScalarVal();
     // For floating point real operands we can directly pass the scalar form
     // to the binary operator emission and potentially get more efficient code.
     if (LHSTy->isRealFloatingType()) {

efriedma-quic

Needs testcase in clang/test/CodeGen/. Needs release note (clang/docs/ReleaseNotes.rst).

efriedma-quic · 2025-11-06T21:26:52Z

clang/lib/CodeGen/CGExprComplex.cpp

      OpInfo.LHS = EmitComplexToComplexCast(LHSVal, LHSTy, OpInfo.Ty, Loc);
  } else {
-    llvm::Value *LHSVal = CGF.EmitLoadOfScalar(LHS, Loc);
+    llvm::Value *LHSVal = CGF.EmitLoadOfLValue(LHS, Loc).getScalarVal();


Do we also need to change the call to EmitStoreOfScalar?

I'll look into it

I think it does make sense to replace it with EmitStoreThroughLValue, what do you think?

That seems fine.

efriedma-quic · 2025-11-10T18:32:03Z

clang/test/CodeGen/2025-11-10-UBSanComplexBitfieldCrash.c

@@ -0,0 +1,10 @@
+// Reduced from https://github.com/llvm/llvm-project/issues/166512
+// RUN: %clang_cc1 %s -emit-obj -std=c23 -fsanitize=bool -o %t


I'd prefer to have a test that actually checks that we generate correct code, not just that we generate code without crashing.

update_cc_test_checks.py is an easy way to write checks.

You're right. I've added a test to verify that the bitfield value is loaded and stored correctly (base commit fails this test)

efriedma-quic · 2025-11-11T19:14:14Z

clang/test/CodeGen/complex-compound-assign-bitfield.c

+int main(void) {
+// CHECK-LABEL: define dso_local i32 @main() #0 {
+    struct Bits b;
+    b.b = b.b += __builtin_complex(-1.0f, 0.0f);


b.b = b.b += is confusing: you can't tell which store the CHECK lines are checking. Can you make the two variables different?

efriedma-quic · 2025-11-12T16:29:49Z

clang/test/CodeGen/2025-11-10-UBSanComplexBitfieldCrash.c

@@ -0,0 +1,10 @@
+// Reduced from https://github.com/llvm/llvm-project/issues/166512
+// RUN: %clang_cc1 %s -emit-obj -std=c23 -fsanitize=bool -o %t


This RUN line is probably going to cause issues in some build configurations, because it's using a backend without specifying a backend and using REQUIRES to check that backend is enabled. Please use -emit-llvm unless there's some reason to check the backend output.

A few CHECK lines here might also be useful, just to confirm the output is what we expect.

~~This test was meant specifically to ensure that the crash in the issue doesn't happen. The crash doesn't happen with -emit-llvm.~~

Edit: I think this is because the instrumentation pass wasn't being run with clang -emit-llvm, so I can just make sure that is run without invoking the backend.

With emit-llvm, it hits an assertion which is essentially fatal; I don't really care whether it crashes with assertions disabled. We can add appropriate CHECK lines which should catch the bad output.

If we're not testing for the assertion failure, this test might not be needed as the code generation is already checked by complex-compound-assign-bitfield.c. What do you think?

I'd be okay with just deleting it, sure.

efriedma-quic

LGTM

efriedma-quic · 2025-11-18T19:08:08Z

There's a merge conflict in the release notes. Let me know when the patch is ready to merge, and I'll merge for you.

github-actions · 2025-11-19T14:12:17Z

🐧 Linux x64 Test Results

111335 tests passed
4417 tests skipped

BStott6 · 2025-11-19T14:21:30Z

@efriedma-quic Should be ready to merge

github-actions · 2025-11-19T19:41:27Z

@BStott6 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

BStott6 added 2 commits November 6, 2025 16:25

[Clang][CodeGen] Use EmitLoadOfLValue instead of EmitLoadOfScalar to …

eab7dd0

…get LHS for complex compound assignment

Merge branch 'llvm:main' into fix-166512

abcef13

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Nov 6, 2025

efriedma-quic reviewed Nov 6, 2025

View reviewed changes

BStott6 added 2 commits November 7, 2025 16:39

Replace EmitStoreOfScalar with EmitStoreThroughLValue too

ab4e0ee

Introduce test for fixed crash

5a203e6

efriedma-quic reviewed Nov 10, 2025

View reviewed changes

BStott6 mentioned this pull request Nov 11, 2025

[TSan] Segfault with target_clones attribute #163369

Closed

BStott6 added 2 commits November 11, 2025 16:38

Add test for correct codegen

6ca51cd

Add release note for fix

1232acd

efriedma-quic reviewed Nov 11, 2025

View reviewed changes

Clarify test

76b16c9

efriedma-quic reviewed Nov 12, 2025

View reviewed changes

Remove UBSan complex bitfield crash test

03d0c67

efriedma-quic approved these changes Nov 18, 2025

View reviewed changes

BStott6 added 2 commits November 19, 2025 12:56

Merge branch 'main' into fix-166512

3cb3f71

Specify triple for codegen test

c85dc55

efriedma-quic merged commit 60a2795 into llvm:main Nov 19, 2025
11 checks passed

		@@ -0,0 +1,10 @@
		// Reduced from https://github.com/llvm/llvm-project/issues/166512
		// RUN: %clang_cc1 %s -emit-obj -std=c23 -fsanitize=bool -o %t

[Clang][CodeGen] Use EmitLoadOfLValue instead of EmitLoadOfScalar to get LHS for complex compound assignment #166798

[Clang][CodeGen] Use EmitLoadOfLValue instead of EmitLoadOfScalar to get LHS for complex compound assignment #166798

Conversation

BStott6 commented Nov 6, 2025

Uh oh!

github-actions bot commented Nov 6, 2025

Uh oh!

llvmbot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BStott6 Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BStott6 Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

efriedma-quic commented Nov 18, 2025

Uh oh!

github-actions bot commented Nov 19, 2025

🐧 Linux x64 Test Results

Uh oh!

BStott6 commented Nov 19, 2025

Uh oh!

Uh oh!

github-actions bot commented Nov 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

llvmbot commented Nov 6, 2025 •

edited

Loading

BStott6 Nov 11, 2025 •

edited

Loading

BStott6 Nov 12, 2025 •

edited

Loading