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

don't require C compiler to codegen or link in CI when binaries won't be tested/used #5669

Closed
wants to merge 3 commits into from

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Dec 14, 2023

On a system with nproc of 16, this brings tests/all_tests compilation from, with -d:debug --opt:none (which uses -0g at the gcc level) i.e. already not optimizing, and not using LTO:

real	4m0.017s
user	21m20.552s
sys	1m1.970s

to

real	2m8.371s
user	2m35.644s
sys	0m22.738s

It should have even more effect with fewer cores, such as the macOS and Windows cloud GitHub Actions CI builders with two cores only, because the nim c -c portion is intrinsically single-threaded regardless so doesn't depend on core count, and this cuts out the parallelizable portion.

Currently, the GitHub Actions CI binary building for the targets it only builds but doesn't run (i.e. make all, in the Build binaries (with trace logging enabled) phase, not make test, the latter of which do run) compile debug/-Og binaries, without LTO, which also aren't run.

It is important to ensure that Nim isn't generating incorrect C code, because definitely has one that regularly (nim-lang/Nim#7593, nim-lang/Nim#18080, nim-lang/Nim#18081, nim-lang/Nim#22888, and nim-lang/Nim#22913 for some C examples, and nim-lang/Nim#22101 for C++). CI should catch this class of build issue.

This PR adjusts this to use -fsyntax-only to avoid the codegen and linking phases.

It calls itself syntax-only, but that implies being able to resolve types sufficiently to figure out the kinds of tokens available, along with other compilation steps, so it would have caught For example,

int main() {
	return x;
}

can be argued to have valid syntax, for a narrow definition of syntax but

c.c: In function ‘main’:
c.c:2:16: error: ‘x’ undeclared (first use in this function)
    2 |         return x;
      |                ^
c.c:2:16: note: each undeclared identifier is reported only once for each function it appears in

because it can't figure out what x is supposed to be.

This should catch all the issues of the sort liked in the Nim issue tracker where Nim generated invalid C code, while requiring half or less of the time for this part of the CI build.

@tersec
Copy link
Contributor Author

tersec commented Dec 14, 2023

Verified with:

proc dummy*[T: SomeNumber](a: T, b: T = 2.5): T =
  result = a

echo dummy(2)

which even with -fsyntax-only repros this issue:

$ nim c --passC:-fsyntax-only --noLinking:on d.nim
Hint: used config file '/etc/nim/nim.cfg' [Conf]
Hint: used config file '/etc/nim/config.nims' [Conf]
..........................................................
CC: ../../usr/lib/nim/lib/std/private/digitsutils.nim
CC: ../../usr/lib/nim/lib/system/dollars.nim
CC: ../../usr/lib/nim/lib/system/io.nim
CC: ../../usr/lib/nim/lib/system.nim
CC: d.nim
.cache/nim/d_d/@md.nim.c: In function ‘NimMainModule’:
.cache/nim/d_d/@md.nim.c:148:36: error: expected expression before ‘)’ token
  148 |         T2_ = dummy__d_6(((NI) 2), );
      |                                    ^
Error: execution of an external compiler program 'gcc -c  -w -fmax-errors=3 -fsyntax-only   -I/usr/lib/nim/lib -o .cache/nim/d_d/@md.nim.c.o .cache/nim/d_d/@md.nim.c' failed with exit code: 1

and from nim-lang/Nim#22101

import std/pegs
discard pegs.peg("")

similarly fails in the same way with -fsyntax-only:

$ nim cpp --passC:-fsyntax-only --noLinking:on --mm:orc --exceptions:goto c.nim
Hint: used config file '/etc/nim/nim.cfg' [Conf]
Hint: used config file '/etc/nim/config.nims' [Conf]
............................................................................
CC: ../../usr/lib/nim/lib/std/private/digitsutils.nim
CC: ../../usr/lib/nim/lib/system/assertions.nim
CC: ../../usr/lib/nim/lib/system/dollars.nim
CC: ../../usr/lib/nim/lib/system.nim
CC: ../../usr/lib/nim/lib/pure/strutils.nim
CC: ../../usr/lib/nim/lib/pure/pegs.nim
CC: c.nim
.cache/nim/c_d/@m..@s..@susr@slib@snim@slib@spure@sstrutils.nim.cpp: In function ‘void nsuAddf(NimStringV2&, NimStringV2, NimStringV2*, NI)’:
.cache/nim/c_d/@m..@s..@susr@slib@snim@slib@spure@sstrutils.nim.cpp:714:57: error: jump to label ‘LA71_’
  714 |                                                         LA71_:;
      |                                                         ^~~~~
.cache/nim/c_d/@m..@s..@susr@slib@snim@slib@spure@sstrutils.nim.cpp:683:137: note:   from here
  683 | (j_2, ((NI) 1), &TM__JGc9b9bh2D3nTdUR7TGyq8aA_33)) { raiseOverflow(); goto LA71_;
      |                                                                            ^~~~~
...

nim-lang/Nim#18429 is found likewise with -fsyntax-only:

$ nim c --passC=-fsyntax-only --noLinking:on min.nim
Hint: used config file '/etc/nim/nim.cfg' [Conf]
Hint: used config file '/etc/nim/config.nims' [Conf]
..........................................................
min.nim(11, 6) Warning: target type is larger than source type [CastSizes]
CC: ../../usr/lib/nim/lib/std/private/digitsutils.nim
CC: ../../usr/lib/nim/lib/system/formatfloat.nim
CC: ../../usr/lib/nim/lib/system/dollars.nim
CC: ../../usr/lib/nim/lib/system/io.nim
CC: ../../usr/lib/nim/lib/system.nim
CC: min.nim
.cache/nim/min_d/@mmin.nim.c: In function ‘NimMainModule’:
.cache/nim/min_d/@mmin.nim.c:190:21: error: assignment to expression with array type
  190 |         LOC3.source = x__min_4.vals;
      |                     ^
Error: execution of an external compiler program 'gcc -c  -w -fmax-errors=3 -fsyntax-only   -I/usr/lib/nim/lib -o .cache/nim/min_d/@mmin.nim.c.o .cache/nim/min_d/@mmin.nim.c' failed with exit code: 1

@tersec tersec changed the title don't require C compiler to codegen or link inCI when binaries won't be tested/used don't require C compiler to codegen or link in CI when binaries won't be tested/used Dec 14, 2023
@tersec tersec marked this pull request as draft December 14, 2023 17:55
Copy link

github-actions bot commented Dec 14, 2023

Unit Test Results

0 files   -          9  0 suites   - 1 101   0s ⏱️ - 27m 32s
0 tests  -   4 226  0 ✔️  -   3 879  0 💤  - 347  0 ±0 
0 runs   - 16 873  0 ✔️  - 16 475  0 💤  - 398  0 ±0 

Results for commit 43efdca. ± Comparison against base commit d669eef.

♻️ This comment has been updated with latest results.

@tersec
Copy link
Contributor Author

tersec commented Dec 15, 2023

Continuing checking against known fails-at-C-compilation stage examples, with 100% correlation so far of works/fails.

nim-lang/Nim#18102:

$ cat t12319.nim
when true:
  proc fn(a, int, b: varargs[int]) =
    echo (a, b)
  fn(1)
$ nim c --passC=-fsyntax-only --noLinking:on t12319.nim
Hint: used config file '/etc/nim/nim.cfg' [Conf]
Hint: used config file '/etc/nim/config.nims' [Conf]
..........................................................
CC: ../../usr/lib/nim/lib/std/private/digitsutils.nim
CC: ../../usr/lib/nim/lib/system/dollars.nim
CC: ../../usr/lib/nim/lib/system/io.nim
CC: ../../usr/lib/nim/lib/system.nim
CC: t12319.nim
.cache/nim/t12319_d/@m..@s..@susr@slib@snim@slib@ssystem@sdollars.nim.c: In function ‘dollar___t4950514957_90’:
.cache/nim/t12319_d/@m..@s..@susr@slib@snim@slib@ssystem@sdollars.nim.c:215:55: error: incompatible type for argument 2 of ‘addQuoted__t4950514957_143’
  215 |         addQuoted__t4950514957_143((&result), x.Field0.Field0, x.Field0.Field1);
      |                                               ~~~~~~~~^~~~~~~
      |                                                       |
      |                                                       NI * {aka long int *}
.cache/nim/t12319_d/@m..@s..@susr@slib@snim@slib@ssystem@sdollars.nim.c:61:114: note: expected ‘tyVarargs__cLNJubUogBf9cxa9butXtOHQ’ but argument is of type ‘NI *’ {aka ‘long int *’}
   61 | 950514957_143)(NimStringDesc** s, tyVarargs__cLNJubUogBf9cxa9butXtOHQ x, NI xLen_0);
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^

.cache/nim/t12319_d/@m..@s..@susr@slib@snim@slib@ssystem@sdollars.nim.c:226:55: error: incompatible type for argument 2 of ‘addQuoted__t4950514957_143’
  226 |         addQuoted__t4950514957_143((&result), x.Field1.Field0, x.Field1.Field1);
      |                                               ~~~~~~~~^~~~~~~
      |                                                       |
      |                                                       NI * {aka long int *}
.cache/nim/t12319_d/@m..@s..@susr@slib@snim@slib@ssystem@sdollars.nim.c:61:114: note: expected ‘tyVarargs__cLNJubUogBf9cxa9butXtOHQ’ but argument is of type ‘NI *’ {aka ‘long int *’}
   61 | 950514957_143)(NimStringDesc** s, tyVarargs__cLNJubUogBf9cxa9butXtOHQ x, NI xLen_0);
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^

Error: execution of an external compiler program 'gcc -c  -w -fmax-errors=3 -fsyntax-only   -I/usr/lib/nim/lib -o .cache/nim/t12319_d/@m..@s..@susr@slib@snim@slib@ssystem@sdollars.nim.c.o .cache/nim/t12319_d/@m..@s..@susr@slib@snim@slib@ssystem@sdollars.nim.c' failed with exit code: 1


.cache/nim/t12319_d/@mt12319.nim.c: In function ‘fn__t4950514957_1’:
.cache/nim/t12319_d/@mt12319.nim.c:111:22: error: incompatible types when assigning to type ‘tyVarargs__cLNJubUogBf9cxa9butXtOHQ’ from type ‘NI *’ {aka ‘long int *’}
  111 |         T2_.Field0 = a;
      |                      ^
.cache/nim/t12319_d/@mt12319.nim.c:112:22: error: incompatible types when assigning to type ‘tyVarargs__cLNJubUogBf9cxa9butXtOHQ’ from type ‘NI *’ {aka ‘long int *’}
  112 |         T2_.Field1 = b;
      |                      ^

@tersec tersec force-pushed the TnR branch 2 times, most recently from 6b62f09 to d4e45a7 Compare January 19, 2024 10:54
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.

1 participant