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

types: explicitly instantiate map_type_impl::deserialize() #22136

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

avikivity
Copy link
Member

@avikivity avikivity commented Jan 1, 2025

The definition of the template is in a source translation unit, but there are also uses outside the translation unit. Without lto/pgo it worked due to the definition in the translation unit, but with lto/pgo we can presume the definition was inlined, so callers outside the translation unit did not have anything to link with.

Fix by explicitly instantiating the template function.

Fixes a bug newly introduced (or rather, a latenct bug that only now has impact), so no backport is needed.

The definition of the template is in a source translation unit, but there
are also uses outside the translation unit. Without lto/pgo it worked due
to the definition in the translation unit, but with lto/pgo we can presume
the definition was inlined, so callers outside the translation unit did not
have anything to link with.

Fix by explicitly instantiating the template function.
@avikivity avikivity added the backport/none Backport is not required label Jan 1, 2025
@avikivity avikivity requested a review from michoecho January 1, 2025 10:15
Copy link
Contributor

@michoecho michoecho left a comment

Choose a reason for hiding this comment

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

@avikivity You already fixed this once, in 27bcd29. But you only fixed it in enterprise, not in master.

@avikivity
Copy link
Member Author

@avikivity You already fixed this once, in 27bcd29. But you only fixed it in enterprise, not in master.

Yes, that's how I knew what to fix. I should have ported the fix to master, but didn't.

@scylladb-promoter
Copy link
Contributor

🟢 CI State: SUCCESS

✅ - Build
✅ - Docker Test
✅ - dtest
✅ - dtest with topology changes
✅ - dtest with tablets
✅ - Offline-installer Artifact Tests
✅ - Unit Tests

Build Details:

  • Duration: 3 hr 45 min
  • Builder: spider2.cloudius-systems.com

@avikivity
Copy link
Member Author

@nyh please review

3 similar comments
@avikivity
Copy link
Member Author

@nyh please review

@avikivity
Copy link
Member Author

@nyh please review

@avikivity
Copy link
Member Author

@nyh please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/none Backport is not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants