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

cloud_storage: topic_recovery_service coro -> continuation for collect_manifest_paths #10853

Merged

Conversation

andijcr
Copy link
Contributor

@andijcr andijcr commented May 18, 2023

arm64 + clang-14 causes a miscompilation of this coroutine (evidence: prefix should be in the form "[0-f]0000000" but it can appear in tests as 0000000.

this commit translate the coroutine code into a chain of futures.

return type is also simplified, since the error result was never set

Fixes #10842

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.1.x
  • v22.3.x
  • v22.2.x

Release Notes

  • none

arm64 + clang-14 causes a miscompilation of this coroutine (evidence:
prefix should be in the form "[0-f]0000000" but it can appear in tests
as 0000000.

this commit translate the coroutine code into a chain of futures.

return type is also simplified, since the error result was never set
@andijcr andijcr force-pushed the fix/rewrite_collect_manifest_paths branch from 9c9b52a to 3adac73 Compare May 18, 2023 10:48
@andijcr andijcr requested review from andrwng, abhijat and jcsp May 18, 2023 10:55
@andijcr andijcr marked this pull request as ready for review May 18, 2023 10:56
@andijcr andijcr added the area/cloud-storage Shadow indexing subsystem label May 18, 2023
@andijcr andijcr changed the title topic_recovery_service: coro -> continuation for collect_manifest_paths cloud_storage: topic_recovery_service coro -> continuation for collect_manifest_paths May 18, 2023
@andijcr
Copy link
Contributor Author

andijcr commented May 18, 2023

@andijcr andijcr merged commit a6741bc into redpanda-data:dev May 18, 2023
@andijcr andijcr deleted the fix/rewrite_collect_manifest_paths branch May 18, 2023 14:38
}

co_return paths;
constexpr static auto hex_chars = std::string_view{"0123456789abcdef"};
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for picking this up!

I wonder if making this constexpr static is enough to avoid the miscompilation, though happy to keep this as is for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so, but i don't know a good way to quickly check so i played safe. Do people fire a arm ci run and decompile it or do they use compiler explored and a good dose of shims?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cloud-storage Shadow indexing subsystem area/redpanda
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI Failure ("binary file matches" via BadLogLines) in EndToEndShadowIndexingTest.test_recover
3 participants