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.

Sign up for GitHub

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

Jump to bottom

POC: Self-contained headers #1423

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

POC: Self-contained headers #1423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e44e8df
Add `tools/check_header.sh` to check whether headers are self-contained
hebasto Sep 11, 2023
652afd3
refactor: Make `field_NxM.h` headers self-contained
hebasto Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,18 @@ jobs:
g++ -Werror include/*.h
clang -Werror -x c++-header include/*.h

headers:
name: "Self-contained headers"
runs-on: ubuntu-latest

steps:
- name: Checkout
uses: actions/checkout@v4

- name: Test script
run: |
./tools/check_header.sh src/field.h src/field_5x52.h src/field_10x26.h

sage:
name: "SageMath prover"
runs-on: ubuntu-latest
Expand Down
7 changes: 0 additions & 7 deletions src/field.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,6 @@
* implementation also provides a secp256k1_fe_verify routine to verify that
* these fields match the run-time value and perform internal consistency
* checks. */
#ifdef VERIFY
# define SECP256K1_FE_VERIFY_FIELDS \
int magnitude; \
int normalized;
#else
# define SECP256K1_FE_VERIFY_FIELDS
#endif

#if defined(SECP256K1_WIDEMUL_INT128)
#include "field_5x52.h"
Expand Down
5 changes: 4 additions & 1 deletion src/field_10x26.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ typedef struct {
* sum(i=0..9, n[i] << (i*26)) < p
* (together these imply n[9] <= 2^22 - 1)
*/
SECP256K1_FE_VERIFY_FIELDS
#ifdef VERIFY
int magnitude;
int normalized;
#endif
} secp256k1_fe;
Comment on lines -33 to 37
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried/considered including field.h instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't it create a circular dependency?

Copy link
Contributor

Choose a reason for hiding this comment

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

In some sense, yes, but it's not a problem because the include guards should break any loops.

If you take that to the extreme, we could also have a single headers.h that includes all internal headers and include headers.h (almost) everywhere. That's a rather simple solution. I'm not convinced that this is very elegant, but it gets the job done...

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not a problem because the include guards should break any loops.

Right. But not for renaming a header to a source file. It is not clear (at least for me) how to make it meaningful. FWIW, I've already tried the testing script that keeps an original header unmodified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. But not for renaming a header to a source file. It is not clear (at least for me) how to make it meaningful.

You mean problems will occur if we move code from a header to a source file?

FWIW, I've already tried the testing script that keeps an original header unmodified.

I don't understand. Can you rephrase?

Copy link
Member Author

@hebasto hebasto Sep 13, 2023 •
edited

Choose a reason for hiding this comment

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

Right. But not for renaming a header to a source file. It is not clear (at least for me) how to make it meaningful.

You mean problems will occur if we move code from a header to a source file?

Yes. I mean:

mv src/field_10x26.h src/field_10x26.c
gcc -c src/field_10x26.c -o src/field_10x26.o

That effectively renders src/field_10x26.h unavailable.

FWIW, I've already tried the testing script that keeps an original header unmodified.

I don't understand. Can you rephrase?

A modified script looks like that:

cp src/field_10x26.h src/field_10x26.c
gcc -c src/field_10x26.c -o src/field_10x26.o


/* Unpacks a constant into a overlapping multi-limbed FE element. */
Expand Down
5 changes: 4 additions & 1 deletion src/field_5x52.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ typedef struct {
* sum(i=0..4, n[i] << (i*52)) < p
* (together these imply n[4] <= 2^48 - 1)
*/
SECP256K1_FE_VERIFY_FIELDS
#ifdef VERIFY
int magnitude;
int normalized;
#endif
} secp256k1_fe;

/* Unpacks a constant into a overlapping multi-limbed FE element. */
Expand Down
16 changes: 16 additions & 0 deletions tools/check_header.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#!/bin/sh

set -u

for header in "$@"; do
source_file=${header%.h}.c
object_file=${header%.h}.o
mv "$header" "$source_file"
gcc -c "$source_file" -o "$object_file"
exit_code=$?
mv "$source_file" "$header"
if [ $exit_code -ne 0 ]; then
exit $exit_code
fi
echo "$header... OK"
done