Fix a crash caused by a bug introduced in C++ overloads support in `GetScopeIdOffset()` #6151

bricknerb · 2025-10-01T13:55:41Z

After this change, we correctly increment the offset by the next switch case type.
Before this change, we accidentally incremented the offset by functions() size instead of cpp_overload_sets() size and vice versa.
Also sorted the switch cases according to the order of the enum, for consistency. This might help prevent a future similar incident.

This fix prevents crashing in the newly introduced test multiple_too_few_args_calls.

This also has the side effect of showing null name for cpp_overload_set_type and cpp_overload_set_value, instead of having an arbitrary name.
Examples that demonstrate the old name is arbitrary can easily be seen in tests like cpp_namespace.carbon and decayed_param.carbon, but careful review would show that all old names are arbitrary, though often luckily almost make sense.

We might want to have a proper name for these, but it's beyond the scope of this crash fixing change.
See #6156.

Part of #5915.

…etScopeIdOffset()` After this change, we correctly increment the offset by the next switch case type. Before this change, we accidentally incremented the offset by `functions()` size instead of `cpp_overload_sets()` size and vice versa. Also sorted the switch cases according to the order of the enum, for consistency which might help prevent a future similar incident. This fix prevents crashing in the newly introduced test `multiple_too_few_args_calls`. This also has the side effect of showing `null name` for `cpp_overload_set_type` and `cpp_overload_set_value`, instead of having an arbitrary name. Examples that deomnstrate the old name is arbitrary can easily be seen in tests like `cpp_namespace.carbon` and `decayed_param.carbon`, but careful review would show that all old names are arbitrary, though often luckily almost make sense.

… a simple type Before this change, we wrongly ignore the decision to generate a thunk for a function with default args by overriding this decision with the fact the return type by itself doesn't require a thunk. This causes not generating a thunk which leads to crashing in lowering. Add tests that show that now thunk is generated in `check` and it no longer crashes in `lower. Based on carbon-language#6151. Follow up of carbon-language#6108.

danakj · 2025-10-01T18:00:42Z

toolchain/sem_ir/inst_namer.cpp

    case ScopeIdTypeEnum::For<SpecificInterfaceId>:
+      offset += sem_ir_->vtables().size();
+      [[fallthrough]];
+    case ScopeIdTypeEnum::For<VtableId>:
      // All type-specific scopes are offset by `FirstEntityScope`.


This looks weird to me. We're using vtables.size for the specific interface? And for Vtable we're using a fixed offset with a comment about specific scopes?

Yes, it looks weird, and I tried to explain the bug fix in the description, but probably not well enough.
See the comment before the switch, each type's offset excludes its own type.
The bug was introduced in #5891, take a look at the code before the bug was introduced:

carbon-lang/toolchain/sem_ir/inst_namer.cpp

Lines 129 to 155 in 64139e5

switch (id_enum) {

case ScopeIdTypeEnum::None:

// `None` will be getting a full count of scopes.

offset += sem_ir_->associated_constants().size();

[[fallthrough]];

case ScopeIdTypeEnum::For<AssociatedConstantId>:

offset += sem_ir_->classes().size();

[[fallthrough]];

case ScopeIdTypeEnum::For<ClassId>:

offset += sem_ir_->vtables().size();

[[fallthrough]];

case ScopeIdTypeEnum::For<VtableId>:

offset += sem_ir_->functions().size();

[[fallthrough]];

case ScopeIdTypeEnum::For<FunctionId>:

offset += sem_ir_->impls().size();

[[fallthrough]];

case ScopeIdTypeEnum::For<ImplId>:

offset += sem_ir_->interfaces().size();

[[fallthrough]];

case ScopeIdTypeEnum::For<InterfaceId>:

offset += sem_ir_->specific_interfaces().size();

[[fallthrough]];

case ScopeIdTypeEnum::For<SpecificInterfaceId>:

// All type-specific scopes are offset by `FirstEntityScope`.

offset += static_cast<int>(ScopeId::FirstEntityScope);

return offset;

)

You can see each type adds the size for next switch case type.
The [[fallthrough]] and the comment above the switch is the key here I think.

This change fixes the code for that original logic.

Does this make sense or do I miss something in your comment?

Yeah, this causes confusion indeed. Maybe refactoring as a follow-up would be beneficial.

Got it thank you. LGTM

Refactoring PR: #6159.

danakj · 2025-10-01T18:01:08Z

toolchain/check/testdata/interop/cpp/namespace.carbon

 // CHECK:STDOUT:     import Cpp//...
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %.0b7: %.6e5 = cpp_overload_set_value @foo [concrete = constants.%empty_struct]
+// CHECK:STDOUT:   %.0b7: %.6e5 = cpp_overload_set_value @<null name> [concrete = constants.%empty_struct]


We seem to have lost all the names?

Right, tried to explain that in the description.
I believe the original names were somewhat arbitrary because we looked at the wrong index, and that what causes the crash that I've fixed here.
You can see names like CallQualified or HasQualifiers.plain before this change, which are clearly wrong.
We can work towards adding proper names, but I think this should not block making this fix so we don't access arbitrary items in the array, which sometimes causes crashes.

FWIW, I'm trying to set the names properly in #6156.

I found this strange as well, so I'd prefer landing #6156 as it brings more context for this change.

Sure, I don't mind #6156 merged without having this one merged, though I find the two PRs somewhat separate (one is fixing a bug that causes a crash, which has a side effect of not showing wrong names and the other is properly setting the names).
Since the crash is blocking other efforts, I'd prefer to merge this one in case #6156 requires more discussions.

danakj · 2025-10-01T18:01:34Z

toolchain/sem_ir/inst_namer.cpp

      [[fallthrough]];
    case ScopeIdTypeEnum::For<CppOverloadSetId>:
-      offset += sem_ir_->cpp_overload_sets().size();
+      offset += sem_ir_->functions().size();


This also looks off to me, indices after (things listed before in the switch) the CppOverloadSetIds aren't being offset by the size of cpp overload sets?

That's correct, see my reply to your other comment.

This is a followup of carbon-language#5891. Based on carbon-language#6151. Part of carbon-language#5915.

danakj · 2025-10-02T14:34:29Z

toolchain/sem_ir/inst_namer.cpp

    case ScopeIdTypeEnum::For<SpecificInterfaceId>:
+      offset += sem_ir_->vtables().size();
+      [[fallthrough]];
+    case ScopeIdTypeEnum::For<VtableId>:
      // All type-specific scopes are offset by `FirstEntityScope`.


Got it thank you. LGTM

…end and maintain Instead of having a switch case with `fallthrough` where each case adds the number of entities of the next id type case, have an array that maps id type to a function that gets the number of the matching entities. This array is less confusing and easier to maintain to avoid bugs like the one fixed in carbon-language#6151. The logic that sums only the entities above the given id is now a common logic so no need to change it when changing the set of ids.

…tNamer::GetScopeIdOffset()` This would hopefully help prevent bugs like the one fixed in carbon-language#6151.

…tNamer::GetScopeIdOffset()` (#6165) This would hopefully help prevent bugs like the one fixed in #6151. See refactoring discussion in #6159.

github-actions bot added the toolchain label Oct 1, 2025

bricknerb marked this pull request as ready for review October 1, 2025 13:59

github-actions bot requested a review from chandlerc October 1, 2025 14:00

bricknerb removed the request for review from chandlerc October 1, 2025 14:04

bricknerb enabled auto-merge October 1, 2025 14:05

bricknerb requested review from danakj and ivanaivanovska October 1, 2025 14:14

bricknerb mentioned this pull request Oct 1, 2025

Fix C++ thunk triggering for functions with default args which return a simple type #6152

Merged

danakj reviewed Oct 1, 2025

View reviewed changes

Merge branch 'main' into few_args

00f6c38

bricknerb requested a review from a team as a code owner October 2, 2025 08:30

bricknerb requested review from dwblaikie and danakj and removed request for a team October 2, 2025 08:30

bricknerb added a commit to bricknerb/carbon-lang that referenced this pull request Oct 2, 2025

Properly set the name for C++ overload set instructions in SemIR

0eb43e4

This is a followup of carbon-language#5891. Based on carbon-language#6151. Part of carbon-language#5915.

bricknerb mentioned this pull request Oct 2, 2025

Properly set the name for C++ overload set instructions in SemIR #6156

Merged

bricknerb added 2 commits October 2, 2025 13:10

Merge branch 'main' into few_args

c7e5704

Update tests following merge.

32072e4

bricknerb added a commit to bricknerb/carbon-lang that referenced this pull request Oct 2, 2025

Properly set the name for C++ overload set instructions in SemIR

e3186a9

This is a followup of carbon-language#5891. Based on carbon-language#6151. Part of carbon-language#5915.

danakj approved these changes Oct 2, 2025

View reviewed changes

bricknerb added this pull request to the merge queue Oct 2, 2025

Merged via the queue into carbon-language:trunk with commit 16999a7 Oct 2, 2025
8 checks passed

bricknerb deleted the few_args branch October 2, 2025 14:54

bricknerb mentioned this pull request Oct 2, 2025

Refactor InstNamer::GetScopeIdOffset() to make it easier to comprehend and maintain #6159

Open

bricknerb added a commit to bricknerb/carbon-lang that referenced this pull request Oct 6, 2025

Add blank lines to group case with the above offset increment in `Ins…

b1ab340

…tNamer::GetScopeIdOffset()` This would hopefully help prevent bugs like the one fixed in carbon-language#6151.

bricknerb mentioned this pull request Oct 6, 2025

Add blank lines to group case with the above offset increment in InstNamer::GetScopeIdOffset() #6165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix a crash caused by a bug introduced in C++ overloads support in `GetScopeIdOffset()` #6151

Fix a crash caused by a bug introduced in C++ overloads support in `GetScopeIdOffset()` #6151

Uh oh!

bricknerb commented Oct 1, 2025 •

edited

Loading

Uh oh!

danakj Oct 1, 2025

Uh oh!

bricknerb Oct 2, 2025

Uh oh!

ivanaivanovska Oct 2, 2025

Uh oh!

danakj Oct 2, 2025

Uh oh!

bricknerb Oct 2, 2025

Uh oh!

danakj Oct 1, 2025

Uh oh!

bricknerb Oct 2, 2025

Uh oh!

bricknerb Oct 2, 2025

Uh oh!

ivanaivanovska Oct 2, 2025

Uh oh!

bricknerb Oct 2, 2025

Uh oh!

danakj Oct 1, 2025

Uh oh!

bricknerb Oct 2, 2025

Uh oh!

danakj Oct 2, 2025

Uh oh!

Uh oh!

Uh oh!

	switch (id_enum) {
	case ScopeIdTypeEnum::None:
	// `None` will be getting a full count of scopes.
	offset += sem_ir_->associated_constants().size();
	[[fallthrough]];
	case ScopeIdTypeEnum::For<AssociatedConstantId>:
	offset += sem_ir_->classes().size();
	[[fallthrough]];
	case ScopeIdTypeEnum::For<ClassId>:
	offset += sem_ir_->vtables().size();
	[[fallthrough]];
	case ScopeIdTypeEnum::For<VtableId>:
	offset += sem_ir_->functions().size();
	[[fallthrough]];
	case ScopeIdTypeEnum::For<FunctionId>:
	offset += sem_ir_->impls().size();
	[[fallthrough]];
	case ScopeIdTypeEnum::For<ImplId>:
	offset += sem_ir_->interfaces().size();
	[[fallthrough]];
	case ScopeIdTypeEnum::For<InterfaceId>:
	offset += sem_ir_->specific_interfaces().size();
	[[fallthrough]];
	case ScopeIdTypeEnum::For<SpecificInterfaceId>:
	// All type-specific scopes are offset by `FirstEntityScope`.
	offset += static_cast<int>(ScopeId::FirstEntityScope);
	return offset;

Fix a crash caused by a bug introduced in C++ overloads support in GetScopeIdOffset() #6151

Fix a crash caused by a bug introduced in C++ overloads support in GetScopeIdOffset() #6151

Uh oh!

Conversation

bricknerb commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Fix a crash caused by a bug introduced in C++ overloads support in `GetScopeIdOffset()` #6151

Fix a crash caused by a bug introduced in C++ overloads support in `GetScopeIdOffset()` #6151

bricknerb commented Oct 1, 2025 •

edited

Loading