Skip to content

Conversation

codefromthecrypt
Copy link
Contributor

FuncType.Results can be nil. This fixes the comparison and backfills relevant tests.

Fixes #3096

@codefromthecrypt
Copy link
Contributor Author

macOS failure looks like the same as #2892 ?

tinygo:ld.lld: error: undefined symbol: _notARealFunction
[19703](https://github.com/tinygo-org/tinygo/runs/8221221002?check_suite_focus=true#step:12:19704)
>>> referenced by /var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tinygo492537698/main.o
[19704](https://github.com/tinygo-org/tinygo/runs/8221221002?check_suite_focus=true#step:12:19705)
failed to run tool: ld.lld
[19705](https://github.com/tinygo-org/tinygo/runs/8221221002?check_suite_focus=true#step:12:19706)
--- FAIL: TestFail (0.00s)
[19706](https://github.com/tinygo-org/tinygo/runs/8221221002?check_suite_focus=true#step:12:19707)
    fail
[19707](https://github.com/tinygo-org/tinygo/runs/8221221002?check_suite_focus=true#step:12:19708)
FAIL

@deadprogram deadprogram requested a review from aykevl September 7, 2022 16:03
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Can you add a test that actually tests for the issue in #3096? It looks like a particular kind of code triggers it, which is not covered in the cgo/testdata/*.go files.

@aykevl
Copy link
Member

aykevl commented Sep 8, 2022

The _notARealFunction error message is noise. It just tests for a linker error, which results in some output on stderr. We should make this less confusing by redirecting stderr.

@deadprogram
Copy link
Member

@codefromthecrypt did you see @aykevl comment on this PR?

#3136 (review)

@codefromthecrypt
Copy link
Contributor Author

@deadprogram thanks I missed it. yeap I can add a second layer test beyond the unit tests. np

codefromthecrypt and others added 2 commits September 21, 2022 08:21
FuncType.Results can be nil. This fixes the comparison and backfills
relevant tests.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Co-authored-by: Ayke <aykevanlaethem@gmail.com>
@codefromthecrypt
Copy link
Contributor Author

@FGadvancer I fixed the bug, but we should have an integration test that makes sure it stays fixed. Can you paste the code that led to this panic? It is better than me wildly guessing.

@codefromthecrypt
Copy link
Contributor Author

It took a while for me to figure out from the stack trace what the bug was, but am over budget doing more than work provided. If someone is interested in raising a PR to obviate this adding the second layer of tests, works for me. If only one layer of tests isn't good enough and this bug is tolerable meanwhile, I also don't mind if someone closes this.

@deadprogram
Copy link
Member

This seems better than it is right now, at the very least. Now squash/merging thank you for the code @codefromthecrypt

@deadprogram deadprogram merged commit 725864d into tinygo-org:dev Sep 26, 2022
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.

3 participants