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

PIC-safe assembly for xorgxrdp #68

Merged
merged 17 commits into from
Nov 17, 2017
Merged

Conversation

mirabilos
Copy link
Contributor

See also: #67

I believe the code to be correct, as the transformations were mechanical (even if done manually), and this compiles on Debian GNU/Linux on i386 just fine (Hurd and GNU/kFreeBSD are expected to follow).

@proski
Copy link
Contributor

proski commented Feb 6, 2017

I made a branch merging #75 , #73 and this PR. yuv2rgb_speed is broken by the changes made in this PR.

@mirabilos
Copy link
Contributor Author

Thank you, it is indeed broken, I see why, and I’m working on a fix.

We use ebp instead of ebx in code that uses it, but these were
already using ebp so push/pop it around (after reviewing that
ebx-now-inner-ebp is indeed reloaded in each y loop).
@mirabilos
Copy link
Contributor Author

@proski this new push should address both your legibility concerns and the crash issue (I tested on an actual i386 system)

Copy link
Contributor

@proski proski left a comment

Choose a reason for hiding this comment

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

Much more readable now, thank you. I'll test it later.

%else
; not ELF
%ifdef PIC
%error Position-Independent Code is currently only supported for ELF
Copy link
Contributor

Choose a reason for hiding this comment

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

Please quote error messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even the nasm manual is inconsistent in whether they quote them, but if it pleases your eyes… sure.

@proski
Copy link
Contributor

proski commented Feb 15, 2017

I was able to test the code on a 32-bit systems. The image is good. The test passes (after rebasing this PR onto the current devel branch).

But I would prefer to have constant data in the .text section. It may be inelegant, but it's safe (the data is write protected), portable (support for non-ELF systems is retained), and simple (no GOT).

@mirabilos
Copy link
Contributor Author

I agree, and I will change it once I get more than a few minutes free time… :| I was ill for a while, and work piled up on the dayjob.

@mirabilos mirabilos changed the title asm ELF PIE PIC-safe assembly for xorgxrdp Mar 25, 2017
@mirabilos
Copy link
Contributor Author

See neutrinolabs/librfxcodec#17 for ongoing discussion; these two PRs are linked.

@jsorg71 jsorg71 merged commit 7ca00bb into neutrinolabs:devel Nov 17, 2017
@mirabilos mirabilos deleted the asm-elf-pie branch December 8, 2017 13:08
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.

3 participants