Improve Debug-Experience and Readability

[manuth]

This PR provides the functionality to basically debug the project, especially using Visual Studio Code, and improves the readability of libips.cpp.

I've done this while having a look at your code in order to learn more about your algorithm and for using it in my own project.

Thanks again for your great help - that's awesome, dude 😄

[Alcaro]

Looks like this is one of your first PRs? Many open source maintainers prefer one PR per change, rather than mashing up five different things like this; merges are all or nothing, even if the maintainer would rather merge only some of those things.

Another common beginner mistake: most maintainers don't like large formatting changes, they make it harder to see what commit changes what. Code style is also a lot more subjective than most other parts of programming, your preferences don't necessarily match the maintainers'. The usual solution is leaving it alone until something else is changed nearby.

Personally I don't care too much, but other projects may be crankier.

I don't have VSCode here, so I can't really test it, I'll just trust the .vscode files work. Except one thing:

+ // Verwendet IntelliSense zum Ermitteln möglicher Attribute.

Let's keep this repo in English, okay?

+if [ "$1" = 'Debug' ]; then
+ MODE=$1
+else
+ MODE='Release'
+fi

make.sh is supposed to create a highly optimized binary (and, judging by the mv flips ~/bin/flips, it's not really designed to work for anyone other than me - but I'm fine with removing that specific line). The unoptimized build is better created with 'make' or 'make CFLAGS=-g', I'd rather not break that.

Though I'll agree it's a bit unclear.

+PROJECT='flips'
+ASSEMBLYNAME=${PROJECT}

Is there ever any reason to rename the binary? If no, configurability that nobody uses isn't really useful. Also nobody except Microsoft calls binaries 'assembly'.

+BINPATH='./bin/'"${MODE}"'/'
+TEMPPATH='./obj/'${MODE}'/'

Having the binary in the root directory feels more convenient to me. Also inconsistent quoting style.

-fprofile-dir='"${TEMPPATH}"'

Hey, I didn't know that exists. Neat. Getting those temp files out of the repo root is indeed an improvement.

Though I'm not sure why the path should be configurable. A hardcoded obj/ is perfectly sufficient.

-enum ipserror ips_study(struct mem patch, struct ipsstudy * study)
+enum ipserror ips_study(struct mem patch, struct ipsstudy *study)

Asterisk at the name is a fairly common coding style, but I prefer it this way.

+ if (read8() != 'P' ||
+ read8() != 'A' ||

The intention is that the read8s should line up. Mine does if tab width is 2, yours requires 4. Should be spaces, they work for everyone.

+#define read24() (patchat += 3, ((patchat[-3] << 16) | (patchat[-2] << 8) | patchat[-1]))

Those macros are such a mess... I should've made some kind of binary stream object, except I originally wrote this thing in C where macros are easier than objects. I only switched to C++ long after I wrote that file. The ips_apply/study distinction is completely useless as well.

+ if (!size)
+ {
+ } //no clue (fix the change detector if changing this)

That looks fairly silly, but the original isn't very good either. The correct fix would be returning an error somewhere. Preferably from ips_study, instead of setting that 'scrambled' flag.

164 -

The comment block above is less tied to the other comments than they are to each other, so I prefer making that clear by having that extra linebreak.

- if (targetlen>=16777216) return ips_16MB;
+ if (targetlen>16777216) return ips_16MB;

Good idea, but if the source file is above 16MB and the target is exactly 16MB, I suspect it'll emit an instruction to truncate to 0 bytes. Better add if (targetlen>=16777216 && sourcelen>targetlen) return ips_16MB; // can't truncate to 16MB as well.

- if (thislen>65535) thislen=65535;
- if (offset+thislen>targetlen) thislen=targetlen-offset;
- if (offset==targetlen) continue;
+ if (thislen > 65535)
+ {
+ thislen = 65535;
+ }
+
+ if (offset + thislen > targetlen)
+ {
+ thislen = targetlen - offset;
+ }
+
+ if (offset == targetlen)
+ {
+ continue;
+ }

Inflating the line count too much harms readability more than it helps.

+ if (byteshere > 8 + 5 || //rle-worthy despite two ips headers
+ (byteshere > 8 && stopat + byteshere == thislen) || //rle-worthy at end of data
+ (byteshere > 8 && !memcmp(&target[offset + stopat + byteshere], &target[offset + stopat + byteshere + 1], 9 - 1))) //rle-worthy before another rle-worthy

I'm not sure if you ran an autoformatter here or are trying to make some weird argument about my line length, but having the comments so far from the code looks pretty silly. Better add a linebreak or two in the memcmp arguments.

Nontrivial PRs often get a few rounds of comments before they're merged, it's nothing unusual. Do you want me to implement most of those comments (everything except VSCode, I don't know German, and while I can guess how to make it use makefile, I can't test it), do you want to do it yourself, or do you think I'm misjudging something?

[manuth]

Looks like this is one of your first PRs? Many open source maintainers prefer one PR per change

You're totally right - this, honestly, is the first time I'm collborating using git in any way - 'til now I've been coding solo :')
I'll remember that and next time I won't provide that many changes in one PR.

+ // Verwendet IntelliSense zum Ermitteln möglicher Attribute.

I really haven't noticed the auto-generated files contain German comments this can be fixed in just a second.

The unoptimized build is better created with 'make' or 'make CFLAGS=-g', I'd rather not break that.

Currently I'm adding either -O0 -g or -O3 according to the $MODE-variable.

+if [ "${MODE}" = "Debug" ]; then
+    FLAGS+=' -O0 -g'
+elif [ "${MODE}" = "Release" ]; then
+    FLAGS+=' -O3'
+fi

Is there some smoother way to do that?

Also nobody except Microsoft calls binaries 'assembly'.

You sure have noticed I'm a clicky-bounty-Microsoft user - changes made to the make.sh are based on MSBuild-defaults - that's why the namings look similar.

Of course these superfluous variables can be replaced.

The intention is that the read8s should line up. Mine does if tab width is 2, yours requires 4. Should be spaces, they work for everyone.

Looks like I messed up my editor settings - usually I have my identation set to 4 spaces instead of tab.

Good idea, but if the source file is above 16MB and the target is exactly 16MB, I suspect it'll emit an instruction to truncate to 0 bytes. Better add if (targetlen>=16777216 && sourcelen>targetlen) return ips_16MB; // can't truncate to 16MB as well.

Understood! It's because in that case the length of the target is added to the end of the patch, right?
In that case, I think, if (sourcelen != targetlen && targetlen >= 0xFFFFFF) would match perfect...?

I'm not sure if you ran an autoformatter

Point for you! Yeah - honestly I first ran an autoformatter over libips.cpp and then added some line breaks and curly braces by myself since gdb seems to just skip expressions that are kept on a single line such as

		while (offset<sourcelen && (offset<sourcelen?source[offset]:0)==target[offset]) offset++;

Do you want me to implement most of those comments (everything except VSCode... )

I honestly don't know what comments you mean, I'm sorry 😅

and while I can guess how to make it use makefile, I can't test it

Well it's actually pretty easy since I've created all configuration-files that are required.
You just need to install the C/C++-Extension available on the Marketplace and press CTRL+B in order to build the project or F5 in order to build the project with -g and -O0-flags set and launch the debugger.

Nontrivial PRs often get a few rounds of comments before they're merged, it's nothing unusual.

Don't you worry, I won't get you wrong. It's real cool you're having a look at the changes I've made and you consider implementing them ^^

[Alcaro]

Is there some smoother way to do that?

As I said, better leave make.sh alone for debug builds, it's highly tuned for optimization. All the -f in FLAGS are various optimizations; compiling twice with profile.sh in between is profiling, which is an optimization. Better call 'make' or 'make CFLAGS=-g' directly from VSCode for debug.

Looks like I messed up my editor settings - usually I have my identation set to 4 spaces instead of tab.

I prefer tab for indentation and space for alignment. Not sure how many IDEs support that.

It's because in that case the length of the target is added to the end of the patch, right?

Yes, IPSes end with the target size if the source is bigger. But we still need the 'target >= 0x01000001' check, IPS can't write beyond 16MB even if the target is bigger.

While it's theoretically possible to emit a 64K-1 block of mostly unchanged data starting at 16M-1, allowing the target to be 16M+64K-2 bytes, Flips doesn't support creating such patches, it prefers you switch to BPS. And even if it was supported, 16M+64K-2 bytes is still a limit that should be checked for. (Feel free to implement that in your patcher and claim you have a feature I don't.)

Not sure if Flips supports applying such a patch, either. It should, but it's untested and may be buggy.

since gdb seems to just skip expressions that are kept on a single line

Ah, so that's what you're doing. Yeah, then linebreaks make sense. Personally I do much of my debugging by spamming print statements all over, but making things easy for gdb is a valid concern. I'll try to limit oneliner if statements in my other projects.

I honestly don't know what comments you mean, I'm sorry 😅

My complaints in my prior comment. Someone has to fix them, your choice which of us.

Well it's actually pretty easy since I've created all configuration-files that are required.

That'd work if I had VSCode installed, but I don't, and I hate installing stuff if I'm not going to use it a lot. I'll just trust that you've tested them properly.

[manuth]

I just fixed the indentation and the code-design itself - I think it should be right the way you want it to be like.

Better call 'make' or 'make CFLAGS=-g' directly from VSCode for debug.

Okay - I've done that now in b44b4f0 and 7e0b310.

Yes, IPSes end with the target size if the source is bigger. But we still need the 'target >= 0x01000001' check, IPS can't write beyond 16MB even if the target is bigger.

While it's theoretically possible to emit a 64K-1 block of mostly unchanged data starting at 16M-1, allowing the target to be 16M+64K-2 bytes, Flips doesn't support creating such patches, it prefers you switch to BPS.

I just tried to adjust the size-restriction until I realized it'd cause bugs if a part after 0x1000000 'd look the same.
I think the current size-restriction I wrote in 1fc8570 should work out perfectly.

Personally I do much of my debugging by spamming print statements all over

To be honest this is the very first C/CPP-project I'm working on - I wouldn't even know how to print stuff to the debug-console 😂

My complaints in my prior comment. Someone has to fix them, your choice which of us.

Alright - I just fixed this in 63781b4. The auto-generated comments are now in English.

That'd work if I had VSCode installed, but I don't, and I hate installing stuff if I'm not going to use it a lot. I'll just trust that you've tested them properly.

Alright - just tested it and it's working just fine!
Now make CFLAGS='-O0 -g' is invoked for debugging and ./make.sh is invoked for building the optimized binary.

[Alcaro]

This PR is starting to get fairly messy.

Okay - I've done that now in b44b4f0 and 7e0b310.

Looks good to me. I'm not completely sure if that one passes the variables you made the makefile expect, but that's better fixed in the makefile. Let's not make VSCode the only way to build Flips. (I think VSCode can call Makefiles without going via the shell, but that doesn't make any real difference.)

I think the current size-restriction I wrote in 1fc8570 should work out perfectly.

Yeah, that looks good.

To be honest this is the very first C/CPP-project I'm working on - I wouldn't even know how to print stuff to the debug-console joy

Print spam debugging, also known as printf debugging, is a common technique in almost all languages. It works just as well in C# as C++.

These parts of the PR are definitely improvements:

• Allow target size 0x01000000
• More linebreaks, rather than oneliners
• More spacing around operators and comments
• More VSCode support
• Putting profiling data and Windows resource object files in a subdirectory
• Splitting the 'rle-worthy before another rle-worthy' condition to multiple lines
• Making make.sh less me-specific by removing mv flips ~/bin/flips

These improve some things, but worsen others, and as far as I can see, the sum is negative; unless you feel I'm wrong (perhaps I am), they should be reverted:

• Making output locations configurable
• Making the Makefile demand BINPATH, PROJECT and a few other variables to exist
• Removing that linebreak near the Flips/LIPS comparison
• Attaching asterisks to the variable names
• -rf on the rm seems unneeded; unless they're known useful, they should be removed, especially if there's variables nearby; bugs there tend to be dangerous

These could be improved, but are suboptimal in the current version too, I won't expect you to fix them (but you may if you want to):

• Tab/space confusion on read8s
• Clarify the relation between make.sh and Makefile
• Size-0 RLE should return ips_invalid, rather than poke w_scrambled (other uses of w_scrambled should remain)
• Too many linebreaks above 'check if RLE here is worthwhile'; two lines per condition is enough, five is excessive

Once the middle list is empty, I'll merge this and fix whatever remains in the third list.

[manuth]

Aite - I think I patched everything...?
The makefile is independent now and doesn't depend on variables such as PROJECT or BINPATH.
Everything else is patched, too

[Alcaro]

oh right, I forgot this one.

All those reversions/etc make it hard to merge, so I copypasted the good parts. Did I miss anything?

[Alcaro]

No answer, I'll just assume everything is fine.