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

all: enables dynamic linking, removes R15 is clobbered #407

Merged
merged 9 commits into from Feb 27, 2023

Conversation

armfazh
Copy link
Contributor

@armfazh armfazh commented Feb 22, 2023

Issue: CIRCL cannot be compiled as a plugin, i.e., cannot be dynamically linked.
Fixes #391

Detection

Adds a CI job that checks whether CIRCL can be compiled using -buildmode=plugin.

When compiling, the compiler shows errors such as:

asm: arith_amd64.s:2450: when dynamic linking, R15 is clobbered by a global variable access and is used here
   ADCQ AX, R15
   MULXQ AX, R15, BX
   MULXQ AX, BX, R15

Why?: The main reason is that Go compiler uses R15 to access to global variables, so it alters its value (do not restore it).

Solution

There are two solutions:

  1. reduce the use of global variables when possible. (In our case, some global variables store constant values, which can be replaced by immediate addressing. e.g. MOV $0x22434, AX)
  2. make sure R15 is not used after accessing a global variable.

Using R15 is still possible if there are no reads to R15 after accessing a global variable. Writes to R15 are allowed.

Note: I detected that MULXQ is incorrectly signaled as clobbered. This is a bug in the compiler (golang/go#58632).

For example: In these two cases, R15 is the output of MULX, and the compiler stops the compilation with error.

   MULXQ AX, R15, BX
   MULXQ AX, BX, R15

In this case R15 is used as an input, so the compiler should stop with error.

   MULXQ R15, AX, BX

Changes

  • Updated assembly functions in all cases in which R15 is clobbered.

The best way to review this PR is going commit by commit.

@armfazh armfazh added the fix-A-bug code that fix a bug label Feb 22, 2023
@armfazh armfazh self-assigned this Feb 22, 2023
@armfazh armfazh changed the title all: enables dynamic linking, removes R15 is clooblered all: enables dynamic linking, removes R15 is clobbered Feb 22, 2023
Copy link
Member

@bwesterb bwesterb left a comment

Choose a reason for hiding this comment

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

Wanted to quickly review this, but it will take me more time.

Makefile Outdated Show resolved Hide resolved
dh/csidh/fp511_amd64.s Outdated Show resolved Hide resolved
dh/csidh/fp511_amd64.s Show resolved Hide resolved
@armfazh
Copy link
Contributor Author

armfazh commented Feb 26, 2023

This time I found another bug in the compiler. (See golang/go#58735)
I have applied a workaround from golang/go#58632 (comment)
and the code changes to make CIRCL to compile as plugin were reduced.

This reduction on the patches indicates that the original code is correct, but it is affected by the compiler bugs.

The only exception was the code for ecc/p384, which required more changes. Other changes are easier to follow.

Copy link
Member

@bwesterb bwesterb left a comment

Choose a reason for hiding this comment

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

Ok, most of it looks fine, but I don't understand some parts of it where you use the AX register seemingly pointlessly. I think I might misunderstand something obvious.

dh/csidh/fp511_amd64.s Show resolved Hide resolved
@@ -29,27 +29,36 @@
// |-128-| x |--- 256 ---| = |------ 384 ------|
// Assuming the first digit multiplication was already performed.
#define MULX128x256(I1, M1, T1, T2, T3, T4, T5) \
MULXQ M1+ 8(SB), T4, T2 \
MOVQ M1+ 8(SB), AX \
MULXQ AX, T4, T2 \
Copy link
Member

Choose a reason for hiding this comment

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

This change (and some a few similar ones below) seem unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's unnecessary, but it is included due to bug in the compiler. (see go/issues/58735). So this is a workaround.

In short, what is happening here is that MULXQ cannot reference to the M1 location because is a global variable.

Copy link
Member

@bwesterb bwesterb Feb 27, 2023

Choose a reason for hiding this comment

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

Ah, makes sense! A note may be nice.

dh/sidh/internal/p434/arith_amd64.s Show resolved Hide resolved
@armfazh armfazh merged commit c2daa95 into cloudflare:main Feb 27, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix-A-bug code that fix a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go plugin not compiling
2 participants