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

nimble build errors on Linux on jtv/v2 branch #122

Open
ee7 opened this issue Feb 19, 2024 · 5 comments
Open

nimble build errors on Linux on jtv/v2 branch #122

ee7 opened this issue Feb 19, 2024 · 5 comments

Comments

@ee7
Copy link

ee7 commented Feb 19, 2024

In the GitHub Actions environment, when trying to run nimble test or nim r tests/test.nim:

/home/runner/work/con4m/con4m/tests/test.nim(3, 18) Error: undeclared identifier: 'toAnsiCode'
candidates (edit distance, scope distance); see '--spellSuggest': 
 (6, 4): 'AtomicNodes'
 (6, 4): 'HtmlNode'
 (6, 4): 'ItalicOff'
       Tip: 6 messages have been suppressed, use --verbose to show them.
tools.nim(36)            doCmd

    Error:  Execution failed with exit code 1
        ... Command: /home/runner/work/con4m/con4m/nimdir/bin/nim c --noNimblePath -d:NimblePkgVersion=0.2.0 --path:/home/runner/.nimble/pkgs2/nimutils-0.2.1-4689d581a064f06b3dbdcb2e60175a93c36797dd --path:/home/runner/.nimble/pkgs2/unicodedb-0.12.0-445[24](https://github.com/crashappsec/con4m/actions/runs/7958493760/job/21723477313#step:5:25)16471e2fe87[26](https://github.com/crashappsec/con4m/actions/runs/7958493760/job/21723477313#step:5:27)eb6070ed6ea7368171cc09 --hints:off -r --path:. /home/runner/work/con4m/con4m/tests/test
@viega
Copy link
Contributor

viega commented Feb 19, 2024

Did you read the con4m doc I sent around yesterday? I wouldn't bother w the dev branch as nothing is the same (including the tests) and that should go away as quickly as we can manage the chalk integration for the v2 branch.

@ee7
Copy link
Author

ee7 commented Feb 19, 2024

Did you read the con4m doc I sent around yesterday?

Yes. And I saw the bit about the new test harness.

I wouldn't bother w the dev branch

OK. Are things in a state where we can work towards getting tests running in CI for the v2 branch?

And is it expected that I currently get an error when running nimble build on the v2 branch?

$ git log -n1 --oneline
a59a292 (HEAD -> jtv/v2, crashappsec/jtv/v2) Remove the old c4m directory, it's not used.
$ nimble build
...
/tmp/nimble_23954/githubcom_crashappsecnimutils_#1b16dfecc54bda0d6492f96dfe4305bad68ec80d/nimutils/c/strcontainer.c: In function ‘c4str_from_file’:
/tmp/nimble_23954/githubcom_crashappsecnimutils_#1b16dfecc54bda0d6492f96dfe4305bad68ec80d/nimutils/c/strcontainer.c:65:34: error: ‘O_EXLOCK’ undeclared (first use in this function); did you mean ‘O_EXEC’?
   65 |     int fd = open(name, O_RDONLY|O_EXLOCK);
      |                                  ^~~~~~~~
      |                                  O_EXEC
/tmp/nimble_23954/githubcom_crashappsecnimutils_#1b16dfecc54bda0d6492f96dfe4305bad68ec80d/nimutils/c/strcontainer.c:65:34: note: each undeclared identifier is reported only once for each function it appears in

Edit: same error in CI for PR #121 (which I rebased on top of the v2 branch). O_EXLOCK and O_SHLOCK aren't portable, but I see this is recent code.

@ee7 ee7 mentioned this issue Feb 19, 2024
4 tasks
@ee7
Copy link
Author

ee7 commented Feb 20, 2024

I updated #120 with crashappsec/nimutils@74130e3 and the latest v2 branch changes.

The next failure for nimble build on Linux in CI:

/usr/bin/ld: /home/runner/.cache/nim/test_r/@merr.nim.c.o: in function `print_con4m_trace':
@merr.nim.c:(.text+0xb9): undefined reference to `getStackTrace'
/usr/bin/ld: /home/runner/.cache/nim/test_r/@merr.nim.c.o: in function `customValidationError__err_u1570':
@merr.nim.c:(.text+0x31c1): undefined reference to `find_string_at'
/usr/bin/ld: /home/runner/.cache/nim/test_r/@[email protected]: in function `foreign_z_call':
@[email protected]:(.text+0x527): undefined reference to `run_0c00l_vm'
collect2: error: ld returned 1 exit status

@viega
Copy link
Contributor

viega commented Feb 20, 2024

(In my last commit) I did what I could without having time to test on Linux to remove potential linker errors. The problem is, I see no reason why run_0c00l_vm wouldn't have linked, so might need someone else to address since I won't have access to my linux box.

@ee7 ee7 changed the title tests: currently fail on dev branch nimble build errors on Linux on jtv/v2 branch Apr 2, 2024
@ee7
Copy link
Author

ee7 commented Apr 2, 2024

(In my last commit) I did what I could without having time to test on Linux to remove potential linker errors. The problem is, I see no reason why run_0c00l_vm wouldn't have linked, so might need someone else to address since I won't have access to my linux box.

For visibility to others: it was because macOS builds enabled LTO, which caused files to be compiled that weren't compiled on Linux. Resolved in #128.

ee7 added a commit that referenced this issue Apr 8, 2024
Before this commit, running `nimble build` on Linux would error at link
time with:

    /usr/bin/ld: /home/runner/.cache/nim/test_r/@Merr@soutput.nim.c.o: in function `customValidationError__errZoutput_u909':
    @Merr@soutput.nim.c:(.text+0x28c1): undefined reference to `find_string_at'
    /usr/bin/ld: /home/runner/.cache/nim/test_r/@[email protected]: in function `foreign_z_call':
    @[email protected]:(.text+0x52d): undefined reference to `run_0c00l_vm'
    collect2: error: ld returned 1 exit status

This happened only when building `test`, and occurred because:

- `find_string_at` is defined in codegen.nim

- `run_0c00l_vm` is defined in vm.nim

and on Linux neither file was compiled when building `test`, which was
an item in `bin` in the nimutils nimble file. From the docs [1]:

    `bin` - A list of files which should be built separated by commas
    with no file extension required. This option turns your package into
    a binary package. Nimble will build the files specified and install
    them appropriately.

`nimble build` did work on macOS, but I believe that's only because
applyCommonLinkOptions() in nimutils enabled
link time optimization (LTO) for macOS only [2]:

    when defined(macosx):
      switch("cpu", targetArch)
      switch("passc", "-flto -target " & targetStr)
      switch("passl", "-flto -w -target " & targetStr &
            "-Wl,-object_path_lto,lto.o")
    elif defined(linux):
      if staticLink:
        switch("passc", "-static")
        switch("passl", "-static")
      else:
        discard
    else:
      echo "Platform not supported."
      quit(1)

which caused files to be compiled that otherwise wouldn't be.

Restructure slightly to allow `nimble build` to succeed without LTO.
This is much better than just enabling LTO on Linux, which dramatically
increases compilation time with a typical setup. It also wouldn't be
ideal for LTO to be _required_, especially for debug builds.

Builds would probably be significantly faster with clang, but I tried
to get clang builds working on Linux, and was unsuccessful with e.g.

    CC=clang nimble build \
      -f \
      --cc:env \
      --clang.exe:musl-clang \
      --clang.linkerexe:musl-clang \
      --passC:-flto \
      --passC:-Wno-implicit-function-declaration \
      --passC:-Wno-int-conversion \
      --passC:-Wno-incompatible-function-pointer-types
      -d:zippyNoSimd

This commit adds a `vm` import in test.nim, and extracts the
find_string_at proc to a separate module. It seemed like it wasn't
possible to simply add some lines like:

    import codegen

or

    from codegen import find_string_at

due to Nim not supporting mutual imports yet, which is the reason for
these importc and exportc in the first place.

Closes: #122

[1] https://nim-lang.github.io/nimble/nimble-reference.html#optional
[2] https://github.com/crashappsec/nimutils/blob/74130e392d9b/nimutils/nimscript.nim#L76-L89
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

No branches or pull requests

2 participants