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

Valgrind fails in CI #790

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Valgrind fails in CI #790

wants to merge 4 commits into from

Conversation

phischu
Copy link
Collaborator

@phischu phischu commented Jan 22, 2025

No description provided.

@marvinborner marvinborner self-assigned this Jan 22, 2025
@phischu phischu force-pushed the fix/ci_valgrind branch 2 times, most recently from cf2c1d9 to 5c19040 Compare January 22, 2025 17:07
@mattisboeckle
Copy link
Contributor

mattisboeckle commented Jan 22, 2025

I did some investigating on these valgrind errors and found that it was complaining about "uninitialized value was created by a heap allocation" when using bytearrays.

If we just initialize the allocated memory to 0 the valgrind errors disappear (at least locally)

phischu and others added 4 commits January 23, 2025 00:05
We were accessing uninitialized memory whenever using bytearrays which causes valgrind to complain.
Now that memory is just initialized to 0
@jiribenes
Copy link
Contributor

I'm all for using calloc in bytearrays :)

@marvinborner marvinborner removed their assignment Jan 22, 2025
@b-studios
Copy link
Collaborator

That would be great! Is there any performance implications of doing that?

@marvinborner
Copy link
Member

marvinborner commented Jan 22, 2025

I'm confused, this shouldn't be a problem, right? Why are we reading from the bytearray when it's not initialized yet (especially in all IO tests)? If I remember correctly, we've discussed calloc-ing before and decided against it(?)

I think the performance implications could be significant, since bytearrays could be used for loading multiple gigabytes of data etc.

@phischu
Copy link
Collaborator Author

phischu commented Jan 23, 2025

It is good to know that initializing the array would fix it, but we should not actually initialize them to zero every time. Rather, we should find the underlying out-of-bounds access. I can not reproduce those errors locally, but @mattisboeckle since you can, please take a look.

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.

5 participants