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

[Semi-regression] C code contains backtick`gensym #23711

Closed
mratsim opened this issue Jun 12, 2024 · 1 comment · Fixed by #23716
Closed

[Semi-regression] C code contains backtick`gensym #23711

mratsim opened this issue Jun 12, 2024 · 1 comment · Fixed by #23716

Comments

@mratsim
Copy link
Collaborator

mratsim commented Jun 12, 2024

As mentioned in Discord at https://discord.com/channels/371759389889003530/768367394547957761/1250440092208730152

Using the latest version2-0 branch I have a regression compared to 2.0.4 that leads to backtick of gensym'ed symbols not being translated to X60 as usual due to #22944 being backported.

See CI run: https://github.com/mratsim/constantine/actions/runs/9471919674/job/26096274691?pr=393#step:23:47

image

It seems to also lead to conflicting types using same name variables
image

https://github.com/mratsim/constantine/actions/runs/9471919674/job/26096274691?pr=393#step:23:13


Reproducing

git clone https://github.com/mratsim/constantine
cd constantine
nim c --cc:clang --outdir:build -d:danger --hints:off --warnings:off --verbosity:0 --threads:on --noMain --app:lib --nimMainPrefix:ctt_init_ --nimcache:nimcache/X60 bindings/lib_constantine.nim

Note that gcc-14 has issues that might not be backported yet so it's easier to use Clang (https://github.com/nim-lang/Nim/pulls?q=is%3Apr+%22gcc+14%22+is%3Aclosed+ )


Similar bug

If it helps, in the past I used to have those issues in v2.0.4, but only in templates and only with {.noInit.} variables.

I found 2 workarounds:

  1. functions see: https://github.com/mratsim/constantine/pull/338/files#diff-9d369fc072ba230397943805ef3e09f4726cef5166ed8774d71657ee0fe0a6c1L1442
    image
  2. Using inject see: https://github.com/mratsim/constantine/pull/346/files#diff-9af16dfbdb13c0086d14ae9290dab7feba9d35fe1e14f3a4f8ca04455aef49b2L68
    image

Unfortunately I tried to minimize the issue to a single file and failed at the time.

@mratsim
Copy link
Collaborator Author

mratsim commented Jun 12, 2024

Also it's important to note that some errors are generated even in files with no push

image

https://github.com/mratsim/constantine/blob/master/constantine/math/extension_fields/square_root_fp2.nim

The z_NEQ variable comes from this template:
https://github.com/mratsim/constantine/blob/6ca7cbf/constantine/platforms/constant_time/ct_routines.nim#L138-L141

image

and the top of the file pushes {.inline.}, which is not applied to templates, and {.raises: [].}

@mratsim mratsim changed the title C code contains backtick`gensym [Semi-regression] C code contains backtick`gensym Jun 12, 2024
@Araq Araq closed this as completed in 646bd99 Jun 19, 2024
narimiran pushed a commit that referenced this issue Jun 24, 2024
Araq pushed a commit that referenced this issue Jul 17, 2024
This adds Constantine to the important packages. Release announcements:
- https://forum.nim-lang.org/t/11935
- https://github.com/mratsim/constantine/releases/tag/v0.1.0

Unfortunately at the moment I'm in a conundrum.

- Constantine cannot compile on devel due to
#23547
- The workaround is changing 
  ```Nim
func mulCheckSparse*(a: var QuadraticExt, b: static QuadraticExt)
{.inline.} =
  ```
  to
  ```Nim
  template mulCheckSparse*(a: var QuadraticExt, b: QuadraticExt) =
  ```
but this does not compile on v2.0.8 due to `gensym` issues despite
#23716

![image](https://github.com/nim-lang/Nim/assets/22738317/21c875d7-512f-4c21-8547-d12534e93a58).
i.e. as mentioned in the issue
#23711 there is another gensym bug
within templates that was fixed in devel but not the v2.0.x series and
that is not fixed by #23716
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant