BinaryAnalysisPlatform / qemu

Official QEMU mirror
Other
9 stars 12 forks source link

Branching strategy #31

Open Rot127 opened 1 year ago

Rot127 commented 1 year ago

I (tried) to rebase the tracewrap-6.20 branch onto the newest stable QEMU and it is not fun. There are a bunch of conflicts.

I suggest we agree on a branch strategy. Because if one person does the rebase and has to fix 2+ architectures every time, we definitely end up with false tracing (because we have no proper tests).

How about we have a main branch which only implements the core of tracewrap.

We can call it tracewrap-X.Y, which is the current newest stable branch of QEMU + one or few more commits with the core tracewrap code. This one is easily updated if QEMU has a new release. It could (should?) be even automated with the CI.

From this every arch branches. If some needs to trace a specific arch they select the archs branch. If it needs a rebase onto the newest tracewrap-X.Y, fine. But since the user will use it anyway, it gets directly tested.

cc @thestr4ng3r @DMaroo Since ARM and x86/i386 had many many conflicts.

Rot127 commented 1 year ago

@XVilka @ivg Would you mind pushing QEMUs latest stable here? I'd like to open a PR onto it with tracewrap.

DMaroo commented 1 year ago

That's a good idea, makes sense to me.

XVilka commented 1 year ago

Idea is interesting but we need also a branch that is sum of all these architecture branches - something like tracewrap-8.2-all

XVilka commented 1 year ago

In the future, as TCG plugins API almost passed the review, I hope it would alleviate the pain by just using the API instead: https://github.com/BinaryAnalysisPlatform/qemu/issues/24

DMaroo commented 1 year ago

Idea is interesting but we need also a branch that is sum of all these architecture branches - something like tracewrap-8.2-all

This would be relatively easy since all the branches for individual archs would (likely) not share any files/code. Hence, no merge conflicts in merging them all to have a combined branch.

Rot127 commented 1 year ago

Hence, no merge conflicts in merging them all to have a combined branch.

The merge conflicts come from our tracewrap additions in the translate.c files. For very active architectures, there are a lot of changes in between releases and then we get again many merge conflicts.

Idea is interesting but we need also a branch that is sum of all these architecture branches - something like tracewrap-8.2-all

Why is this necessary? Some architectures have no additions for several releases. For them it is not worth the effort to rebase them onto the newest one IMHO. So where would be the advantage of having a single all containing branch?

XVilka commented 1 year ago

Why is this necessary? Some architectures have no additions for several releases. For them it is not worth the effort to rebase them onto the newest one IMHO. So where would be the advantage of having a single all containing branch?

It's not uncommon for QEMU to perform tree-wide changes to the TCG uplifters. Recent examples are atomicity changes, new negsetcond opcode:

thestr4ng3r commented 1 year ago

We can call it tracewrap-X.Y, which is the current newest stable branch of QEMU + one or few more commits with the core tracewrap code.

I propose tracewrap-X.Y-base to clearly indicate this is only the base with no added usable tracing by itself.

Idea is interesting but we need also a branch that is sum of all these architecture branches - something like tracewrap-8.2-all

Wouldn't this be essentially the same as we have now, i.e. one branch with everything in it?

I am not yet fully convinced that the proposed approach will have more benefits (independent and thus easier rebasing per architecture) than downsides (more branches to handle, having to merge back core changes from architecture branches if there are any), but it could be a worthwhile experiment to try this approach for a new rebase to see how well it works in practice.

DMaroo commented 1 year ago

For very active architectures, there are a lot of changes in between releases and then we get again many merge conflicts.

Agreed, but there won't be any inter-arch merge conflicts (since each of those translate.c files is under its own arch directory), which is what I meant in that comment. So we could have a combined branch by merging all the separate arch branches without any conflicts.

We would just need to make sure that all the individual branches are conflict free and up to date, then the combined branch would also be conflict free and up to date.

Rot127 commented 1 year ago

We would just need to make sure that all the individual branches are conflict free and up to date,

Yes. This is the work intensive part. If we can agree that everyone rebases their branch on top on the newest QEMU release, we are good.

XVilka commented 1 year ago

@Rot127 I pushed the updated master and stable-8.1 from the latest QEMU

ivg commented 10 months ago

I have no objections to the proposed branching strategy. However, once we finalize it, let's write it in some document, either a wiki page or in a CONTRIBUTING.md file, your choice.