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

improvement of the magic breakpoints fix #55 #58

Merged
merged 1 commit into from
Aug 20, 2023
Merged

improvement of the magic breakpoints fix #55 #58

merged 1 commit into from
Aug 20, 2023

Conversation

therealdreg
Copy link
Member

@therealdreg therealdreg commented Aug 20, 2023

A good start from PR #55

Just implemented support for various types of magic breakpoints, along with the ability to modify them at runtime from within the debugger. So, the Windows XP NTLDR will no longer be a problem.

fix #55

windows xp ntldr have code like this:

0000000000020252: (                    ): mov cr0, eax              ; 0f22c0
0000000000020255: (                    ): xchg bx, bx               ; 87db
0000000000020257: (                    ): nop                       ; 90

And this code is called a lot of times!

ring3 code CANT execute OUT instruction (0x8AE0)

So the ring3-dev cant use magic breakpoints on Bochs debugger (on first instance)

with this PR, the user can select what register should breaks:

#=======================================================================
# MAGIC_BREAK:
# This enables the "magic breakpoint" feature when using the debugger.
# The useless cpu instructions XCHG %REGW, %REGW causes Bochs to enter the
# debugger mode. This might be useful for software development.
#
# You can specify multiple at once:
#
# cx dx bx sp bp si di
#
# Example for breaking on "XCHGW %DI, %DI" or "XCHG %SP, %SP" execution
#   magic_break: enabled=1 di sp
#
# If nothing is specified, the default will be used: XCHGW %BX, %BX 
#   magic_break: enabled=1
#
# Note: Windows XP ntldr can cause problems with XCHGW %BX, %BX
#=======================================================================
# magic_break: enabled=1

This PR is 100% backward compatibility

Added a new command to change from debugger the mask of registers, example adding XCHGW %DI, %DI or XCHGW %BX, %BX

setmagicbps "di bx"

clear command:

clrmagicbps "di bx"

@stlintel @vruppert do you like it?

@therealdreg
Copy link
Member Author

therealdreg commented Aug 20, 2023

@stlintel more suggestions?

btw, I prefer using "{ }" like:

for (int i = 1; i < 8; i++) {
    if (strstr(str, magic_bp_regs[i]) != NULL) {
      new_mask |= (1 << i);
    }
  }

Because I've noticed security bugs (and normal bugs) associated when devs dont use {}, but I can use your style if you prefer, just tell me.

I also prefer to practice defensive programming and write conditions like if (0x2A == variable), tell me if this is a problem

I've also made this global:

static const char* const magic_bp_regs[] = { "ax" /* not accessible */, "cx", "dx", "bx", "sp", "bp", "si", "di" };

And enhance security slightly by using const char const*

I also prefer to be explicit in comparisons like (its my taste):

if (strstr(str, magic_bp_regs[i]) != NULL) {

However, I can modify this and simply use:

if (strstr(str, magic_bp_regs[i]))

Tell me what I should do in the future

bochs/bx_debug/dbg_main.cc Outdated Show resolved Hide resolved
@therealdreg
Copy link
Member Author

Thank you @stlintel , I'll try not to make the same mistakes in the next PR, and be more respectful of your time and patience!

@stlintel
Copy link
Contributor

Hi,

The ideal behavior with github was that once project is approved - you could just merge by itself by pressing merge button in the PR. Or you could set 'Enable auto-merge' in the PR and will be merged automatically.
I don't know if the merge button is available for you, some people told it is not available ...
How does it look on your side ?

I will merge it on the evening together with lex and yacc files.
For now - let's see if we can make github interface work for you and others !

Thanks,
Stanislav

@therealdreg
Copy link
Member Author

Hi,

The ideal behavior with github was that once project is approved - you could just merge by itself by pressing merge button in the PR. Or you could set 'Enable auto-merge' in the PR and will be merged automatically. I don't know if the merge button is available for you, some people told it is not available ... How does it look on your side ?

I will merge it on the evening together with lex and yacc files. For now - let's see if we can make github interface work for you and others !

Thanks, Stanislav

I think I don't have privileges to do it, can you give me privileges in the repository?

By the way, don't worry, I will never merge anything without your approval, and I'll do everything via PR

bochs/bx_debug/parser.y Show resolved Hide resolved
bochs/bx_debug/parser.y Show resolved Hide resolved
@stlintel
Copy link
Contributor

I think I don't have privileges to do it, can you give me privileges in the repository?

I an still new to github interface - I wonder how to make it available to everybody.
So once approved PR will be possible to merge without me.

By the way, don't worry, I will never merge anything without your approval, and I'll do everything via PR

@therealdreg
Copy link
Member Author

therealdreg commented Aug 20, 2023

I think I don't have privileges to do it, can you give me privileges in the repository?

I an still new to github interface - I wonder how to make it available to everybody. So once approved PR will be possible to merge without me.

By the way, don't worry, I will never merge anything without your approval, and I'll do everything via PR

I have no idea about GIT or GITHUB, but tell me what you want me to try, and we'll figure out if it works,

@therealdreg
Copy link
Member Author

therealdreg commented Aug 20, 2023

@stlintel If what you're thinking is that anyone can merge at any time, that's madness; they could break the repository, and this can cause a lot of conflicts...

Someone with judgment must orchestrate the merge, perform the rebases, or do whatever is necessary to ensure that the history and main branch is OK

For example, imagine the following:

You approve a PR from JOHN's branch that modifies the config.cc file on line 23.
You approve a PR from ANA's branch that also modifies the config.cc file on line 23.

Both have merge privileges... so now what?

Who resolves the conflict?

And how???

Are you going to let them break the repository / rewriting main history or introduce bugs? Nah

This flow dont makes sense, you have to think about this as if it were a multithreading system xD

btw, I can help you maintain a clearer and cleaner main history (I really don't like merge messages):

image

@stlintel
Copy link
Contributor

github handles the mess.

  • I approve both PRs. Let's even assume they both have Auto-Merge enabled.
  • One will merge first, don't care which one
  • Second will fail to merge to to merge conflict. Author will be required to fix the conflict and get re-approval.
  • Once re-approved commit will merge.

I don't know, this is how I used to do at work. There is still a lot to set up in github but for sure it could work super-nicely.

@therealdreg
Copy link
Member Author

therealdreg commented Aug 20, 2023

github handles the mess.

  • I approve both PRs. Let's even assume they both have Auto-Merge enabled.
  • One will merge first, don't care which one
  • Second will fail to merge to to merge conflict. Author will be required to fix the conflict and get re-approval.
  • Once re-approved commit will merge.

I don't know, this is how I used to do at work. There is still a lot to set up in github but for sure it could work super-nicely.

The problem is the same...

as I said ... someone with judgment must orchestrate the merge, perform the rebases, or do whatever is necessary to ensure that the history and main branch is OK

think in this another scenario:

You approve a PR from JOHN's branch that modifies the config.cc
You approve a PR from ANA's branch that also modifies the dbg_main.cc

Here we don't have a Git conflict, but we might encounter a bug that only a human could detect. What happens if you've approves multiple PRs that may seem good 'isolated' or unrelated, but could cause a bug together? Only a human with a big picture in mind can catch something like this... because you are merging in order etc.. So only you can recognize this kind of errors when merging because you know the project well and understand the consequences and side effects.

Contributors might only be familiar with their part, and not everything that's going on in parallel

This is a philosophical matter, indeed

In my opinion, only experienced developers with good judgment and deep understand of the project should be able to merge

btw, @stlintel your mention

I used to do at work

This is key, for your work its very useful this flow... but Bochs its huge and complex.. the contributors like me only knows a little part.. its not the same context IMO

@stlintel
Copy link
Contributor

This is key, for your work its very useful this flow... but Bochs its huge and complex.. the contributors like me only knows a little part.. its not the same context IMO

At work the project is way more complex than Bochs, which includes dozens of modules with more than 2M of lines of code.
And these problems can occur there and even do occur, but they are VERY rare.
In the other hand, by merging every branch personally I obviously create a bottleneck.
I could review branch but also other admins could. Volker and hopefully in future, you too.
And if there is an issue -> it could be fixed or entire PR could be reverted.

In my opinion, only experienced developers with good judgment and deep understand of the project should be able to merge

This is basically the same. Only experienced developers with good judgment are able to approve.
And they should see all PRs and foresee such conflicts.

@therealdreg
Copy link
Member Author

This is key, for your work its very useful this flow... but Bochs its huge and complex.. the contributors like me only knows a little part.. its not the same context IMO

At work the project is way more complex than Bochs, which includes dozens of modules with more than 2M of lines of code. And these problems can occur there and even do occur, but they are VERY rare. In the other hand, by merging every branch personally I obviously create a bottleneck. I could review branch but also other admins could. Volker and hopefully in future, you too. And if there is an issue -> it could be fixed or entire PR could be reverted.

In my opinion, only experienced developers with good judgment and deep understand of the project should be able to merge

This is basically the same. Only experienced developers with good judgment are able to approve. And they should see all PRs and foresee such conflicts.

Oka, so if you need something just ping me!

@stlintel stlintel enabled auto-merge (squash) August 20, 2023 15:16
@stlintel stlintel merged commit 244033e into bochs-emu:master Aug 20, 2023
1 check passed
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.

magic breakpoints problem on XP ntldr
2 participants