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

Fix SnoopCompile's snoopl macro and add test. #43634

Merged
merged 2 commits into from
Jan 2, 2022
Merged

Fix SnoopCompile's snoopl macro and add test. #43634

merged 2 commits into from
Jan 2, 2022

Conversation

NHDaly
Copy link
Member

@NHDaly NHDaly commented Jan 2, 2022

Fix SnoopCompile's snoopl macro and add test.

Previously the @snoopl functionality from SnoopCompile wasn't unit
tested at all, and so it was broken in #38160,
which changed the requirements for exporting C functions from the
julia-internal shared lib.

This commit restores the functionality by exporting it correctly, and
also adds a unit test for snoopl, to make sure it isn't broken in the
future. Sorry that we didn't test it in the first place! :)

Thanks for pointing this out, @timholy. Fixes timholy/SnoopCompile.jl#270.

@NHDaly
Fix SnoopCompile's snoopl macro and add test.
34844e6 
Previously the `@snoopl` functionality from SnoopCompile wasn't unit
tested at all, and so it was broken in #38160,
which changed the requirements for exporting C functions from the
julia-internal shared lib.

This commit restores the functionality by exporting it correctly, and
also adds a unit test for snoopl, to make sure it isn't broken in the
future. Sorry that we didn't test it in the first place! :)
@NHDaly NHDaly requested a review from timholy January 2, 2022 15:00
@DilumAluthge
Copy link
Member

We would like to eventually replace all uses of tempname in the Julia test suite with mktemp. See #43597 for details.

In your test, could you use mktemp instead of tempname?

@NHDaly
Copy link
Member Author

NHDaly commented Jan 2, 2022

Happily, thanks. I just copy/pasted from the test above it. Should i change that one, too, as part of this PR?

@DilumAluthge
Copy link
Member

That would be great!

@NHDaly
Copy link
Member Author

NHDaly commented Jan 2, 2022

Done! :) Thanks Dilum.

@DilumAluthge
Copy link
Member

FWIW you don't necessarily have to use the do-block notation. If you do e.g. path, io = mktemp(), then Julia will automatically delete the file when the Julia process exits.

That would allow you to avoid those indenting changes.

@NHDaly
Copy link
Member Author

NHDaly commented Jan 2, 2022

That's true, but i actually do think the block indentation is easier to read anyway. :) Thanks tho!

@NHDaly NHDaly mentioned this pull request Jan 2, 2022
Closed
@NHDaly NHDaly requested a review from oscardssmith January 2, 2022 17:03
@oscardssmith oscardssmith added the docs This change adds or pertains to documentation label Jan 2, 2022
@vchuravy vchuravy merged commit 543971f into master Jan 2, 2022
@vchuravy vchuravy deleted the nhd-snoop-llvm-fix-1.8 branch January 2, 2022 21:05
@timholy timholy mentioned this pull request Jan 3, 2022
Closed
vtjnash
vtjnash reviewed Jan 7, 2022
View reviewed changes
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

This should have failed on CI, since there is no jl_dump_emitted_mi_name_fallback method defined. Why did it not fail?

@vtjnash vtjnash mentioned this pull request Jan 7, 2022
Closed
@NHDaly
Copy link
Member Author

NHDaly commented Jan 10, 2022

Why did it not fail?

Yeah, agreed it should have, but it didn't fail because there weren't any tests at all that exercised the functionality until this PR. 😞 Sorry about that! In this PR, i've both fixed it, and added tests so it doesn't break again 👍

@NHDaly
Copy link
Member Author

NHDaly commented Jan 10, 2022

There were tests for it in the SnoopCompile.jl package, just not any tests here. So the failure was discovered in that package: timholy/SnoopCompile.jl#270

@vtjnash
Copy link
Member

vtjnash commented Jan 12, 2022

Jeff already fixed it in #43706

@NHDaly
Copy link
Member Author

NHDaly commented Jan 12, 2022

Ohhhh I misunderstood! You're saying THIS pr should have failed, cause I didn't do it right! 😅

Whoops. Thanks both Jameson and Jeff! 👍👍

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@NHDaly @LilithHafner
Fix SnoopCompile's snoopl macro and add test. (JuliaLang#43634)
157e296 
* Fix SnoopCompile's `snoopl` macro and add test.

Previously the `@snoopl` functionality from SnoopCompile wasn't unit
tested at all, and so it was broken in JuliaLang#38160,
which changed the requirements for exporting C functions from the
julia-internal shared lib.

This commit restores the functionality by exporting it correctly, and
also adds a unit test for snoopl, to make sure it isn't broken in the
future. Sorry that we didn't test it in the first place! :)
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@NHDaly @LilithHafner
Fix SnoopCompile's snoopl macro and add test. (JuliaLang#43634)
b66f12a 
* Fix SnoopCompile's `snoopl` macro and add test.

Previously the `@snoopl` functionality from SnoopCompile wasn't unit
tested at all, and so it was broken in JuliaLang#38160,
which changed the requirements for exporting C functions from the
julia-internal shared lib.

This commit restores the functionality by exporting it correctly, and
also adds a unit test for snoopl, to make sure it isn't broken in the
future. Sorry that we didn't test it in the first place! :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@oscardssmith oscardssmith oscardssmith approved these changes

@timholy timholy Awaiting requested review from timholy

Assignees
No one assigned
Labels
docs This change adds or pertains to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LLVM snooping broken on nightly
5 participants
@NHDaly @DilumAluthge @vtjnash @oscardssmith @vchuravy