Skip to content

fix redis signal handle failure bug (fixed by mhomidi Hadi Omidi) #1326

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

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

QuarkContainer
Copy link
Owner

No description provided.

@QuarkContainer QuarkContainer changed the title fix redis signal handle failure bug (fixed by Hadi Omidi) fix redis signal handle failure bug (fixed by mhomidi Hadi Omidi) Jul 12, 2024
@@ -1310,7 +1310,6 @@ impl Task {
let signo = info.Signo as u64;
let currTask = Task::Current();
let regs = currTask.GetPtRegs();
*regs = PtRegs::default();
Copy link
Collaborator

Choose a reason for hiding this comment

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

that means, we are inheriting saved registers from the last interrupt, instead of setting them up from scratch? do you have idea what specific registers are necessary here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I have no idea. This is also the behavior of X86 code and I copied from somewhere before. Looks golang signal process relies on that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think reusing the whole trap frame may be a foot gun. Time to read the friendly manual!

For now, I think we may keep this PR open for a while as a temporary fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably we just need to keep the register that store the address of g and m structure in the memory in go-runtime. BTW, I agree with @shrik3 and we can keep this PR open for temporary fix.

Copy link
Collaborator

@chl337 chl337 Jul 17, 2024

Choose a reason for hiding this comment

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

@shrik3 If I understand it correctly, for arm64, go initially saves g (Gs,Ms,Ps) in R12. So you don't want to lose that one.

P.S. It may also move it again, before calling sigtrampgo, but I haven't checked it...

@chl337
Copy link
Collaborator

chl337 commented Jul 27, 2024

@QuarkContainer Please change the commit message to something more informative, e.g.

aarch64: preserve the GPR state - Linux ABI

otherwise it's O.K.

@shrik3
Copy link
Collaborator

shrik3 commented Aug 19, 2024

I dig it, we should not "reset" the pt_regs on signal delivery. The kernel code doesn't do that. I don't know why it is the case for x86.

I'll go ahead and merge this.

@QuarkContainer FYI I've rewritten the commit message for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants