Skip to content

Conversation

@jrwrigh
Copy link
Collaborator

@jrwrigh jrwrigh commented Oct 6, 2024

For loop would read past string bounds. Caught using ASAN with CUDA backends.

The loop idea is to use NULL byte at the end of first_dot as the for loop stop, but also copy that NULL byte. So the difference between the "check" byte and the "copy" byte should be 1, not 3.

Side note: I could only get ASAN and CUDA to cooperate with export ASAN_OPTIONS=protect_shadow_gap=0, see google/sanitizers#629 (comment)

@jedbrown
Copy link
Member

jedbrown commented Oct 6, 2024

Good find. Should we enable this flag in CI?

@jrwrigh
Copy link
Collaborator Author

jrwrigh commented Oct 6, 2024

We'd need to add an ASAN job for CUDA or HIP. I guess we'd probably want to make that a separate job, possibly just testing the core library rather than everything (really just want to get ASAN over the JIT functionality).

@jedbrown
Copy link
Member

jedbrown commented Oct 7, 2024

Does it make a time difference to use ASAN in GPU jobs? It might be negligible.

@jrwrigh
Copy link
Collaborator Author

jrwrigh commented Oct 8, 2024

Doing just the fluids-navierstokes tests, it's about a 3% increase in time. Worth it?

@jeremylt
Copy link
Member

jeremylt commented Oct 8, 2024

I'd in that case turn it on only for CUDA since HIP is very slow already

@jedbrown
Copy link
Member

jedbrown commented Oct 8, 2024

I think 3% is acceptable for CUDA or even for both (there are probably other places we could recoup that 3%).

@jeremylt
Copy link
Member

jeremylt commented Oct 8, 2024

For perspective - ROCM CI takes 23ish min while CUDA CI takes 8ish min

@jedbrown
Copy link
Member

jedbrown commented Oct 8, 2024

Let's do it on CUDA only. Do you know about how much of the slower ROCm time is attributable to the weaker GPU versus slower RTC?

@jeremylt
Copy link
Member

jeremylt commented Oct 8, 2024

Unclear. I know I dropped about 15% or so of the CI time on the ROCM job by more aggressively caching the libCEED Basis and Restriction objects, so the RTC is definitely a noticeable factor.

@jrwrigh
Copy link
Collaborator Author

jrwrigh commented Oct 8, 2024

New CI should be separate PR, right? If so, this PR is good to go (as soon as the testing finishes up).

@jeremylt
Copy link
Member

jeremylt commented Oct 8, 2024

I'm good with splitting the CI changes out into a followup

@jrwrigh jrwrigh merged commit dfc3c7d into main Oct 8, 2024
23 of 24 checks passed
@jrwrigh jrwrigh deleted the jrwrigh/fix_oob branch October 8, 2024 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants