Q: How to test correctness of patches against asm files

[mirabilos]

Hi,

I’ll need to patch the following files…

• src/x86/rfxcodec_encode_dwt_shift_x86_sse2.asm
• src/x86/rfxcodec_encode_dwt_shift_x86_sse41.asm

… in order to make them usable when compiled with PIC (shared library) or PIE (relocatable binary using PIC mechanism) without having a TEXTREL (which has bad security implications).

For this, I’ll require use of the ebx register (which stores the address of the GOT (global object table) with PIC code on i386), so I have to rewrite part of the code. How can I test correctness of my changes (in both (ELF) PIC and nōn-PIC modes) afterwards?

[proski]

There is an existing test (rfxcodectest.c) which should exercise those functions. You may want to add extra checks. It's very important to make sure that the test fails if the assembly code is incorrect. That way you know your code is executed.

To check PIC code, make sure the code you are testing is in a shared library. To check non-PIC more, use static linking. I suggest that you always write PIC code, it's easier to have one version.

[mirabilos]

The test doesn’t help, as its scope seems to be a speed test, not a test for the correctness of the transformations.

No, we cannot always write PIC code, because the environment may be not a shared library or PIE binary (it can even be a nōn-ELF binary, although I only added ELF PIC for now).

I am fairly certain that the changed code will, at the very least, not crash; I just wonder about the correctness. (Especially as I discovered several pieces in the code that can be optimised further, but I stuck with mechanical transformations to enable ELF PIC for now.)

You can inspect my preliminary patches (uploaded to Debian “experimental” for actual user testing) here:

• https://anonscm.debian.org/cgit/pkg-remote/xrdp.git/tree/debian/patches/i386-pic-asm-part1.diff?h=experimental
• https://anonscm.debian.org/cgit/pkg-remote/xrdp.git/tree/debian/patches/i386-pic-asm-part2.diff?h=experimental

[proski]

There is no way to avoid having a test if correctness needs to be checked. Maybe @jsorg71 has some tests, he is the code author. If not, maybe there is a description of the functions somewhere?

[proski]

Can you provide some context what issue you are trying to fix? What OSes/distros/configurations are affected?

[mirabilos]

I’m fixing that the assembly code cannot be used in PIC (such as libxorgxrdp.so) or PIE (executables) modules; this affects all ELF platforms where PIE is used for executables and/or librfxcodec is built as shared library and/or where X.org uses *.so format for modules instead of classic XFree86® *.a format.

[proski]

So, it should not affect the current xrdp builds, as they use the static library. Once the shared library is used, it would be a problem.

As for xorgxrdp, it would be affected now, as it's compiled to modules.

Is that how you see the issue?

But the way, there is non-assembly fallback for all functions. You can use it to understand the expected functionality.

[mirabilos]

No, current xrdp builds are also affected since GCC now defaults to PIE.

I know about the non-assembly fallback (it’s used on all other architectures, like x32, anyway), but that doesn’t mean I understand that code.

[jsorg71]

Should I add a sanity check encode of one tile for test?

[proski]

Maybe a bitmap file could be added to the repository. Then rfxcodectest could be used to encode the bitmap. The checksum of the result could be compared to the expected checksum.

[jsorg71]

I added rfxencode with #21 I can put test data on server1.xrdp.org that we can wget. Actually, I can put a huge test data set up there for extended testing.

[proski]

I would rather have a small dataset that would cover most issues addressed in the code. We could use the same image converted to different color depths and different dimensions.