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

When should we run tests with address sanitizer? #11

Open
elyashiv opened this issue Jul 21, 2020 · 10 comments
Open

When should we run tests with address sanitizer? #11

elyashiv opened this issue Jul 21, 2020 · 10 comments

Comments

@elyashiv
Copy link

Should we run every exercise with address sanitizer or would it confuse the new students?

Can we filter the results or add a comment to explain whats happening? Can we run the address sanitizer just after a certain exercise/just on exercises that really need allocation?

@siebenschlaefer
Copy link

IMHO we should definitely run the address sanitizer.
I've see too many solutions with memory leaks or out-of-bounds accesses.
I think the question is: How do we present that to the students?

@wolf99
Copy link
Contributor

wolf99 commented Oct 20, 2021

iHiD has indicated that this should be part of the analyzer.
#10 (comment)
However I think the C track does not have an anlyzer yet. Also not concept exercises.

I think getting either of those in place is a pre-requirement to adding more testing.
Could really do with some contributions to get concept exercises added as I have not had enough time to really focus enough to get started with that.

EDIT:

I see on slack, that @iHiD and @ErikSchierboom have now recommended adding this in the test-runner.
Double-checking there, but otherwise, please go ahead and add it here.
Would still also dearly like anyone to contribute concept exercises also! 😉

@iHiD
Copy link
Member

iHiD commented Oct 20, 2021

I'm pretty agnostic to where things live as long as someone working via the CLI can have the same testing experience as someone using the website. So as long as a student can also run it locally (and are told to do so) as part of the test instructions, then I'm happy for it to be in the test runner :)

@ErikSchierboom
Copy link
Member

So as long as a student can also run it locally (and are told to do so) as part of the test instructions, then I'm happy for it to be in the test runner :)

☝️ Should this not be possible, I think the sanitizer should be part of the analyzer.

@siebenschlaefer
Copy link

Currently each makefile has the targets test and memcheck.
test compiles the solution, the tests, and the test framework, links them and executes the tests. Nothing fancy.
memcheck does the same as test but with extra flags for the compiler and linker that enable the AddressSanitizer.

In many of the over 800 solutions I've mentored I found memory leaks or out-of-bounds reads, and I frequently told my students to run make memcheck.

Now that I think about it: Why is memcheck an extra target? Can't we just always use the AddressSanitizer?

@ErikSchierboom
Copy link
Member

Can't we just always use the AddressSanitizer?

To me, that sounds like a very good plan.

@wolf99
Copy link
Contributor

wolf99 commented Oct 21, 2021

We can, but I second the request from #10

If it was explained first for the student, ... it would be much more useful. ... "here's how C lets you do dangerous things with memory, here's why you don't want to do that, and here is a tool (ASAN) that we'll now run on the exercises to make sure those problems are caught".

I think tat for a novice, seeing a stack trace for the first time it is not so easy to figure what the tool is trying to tell you, never mind what it is they need to do about it. Track documentation will be key.

@ryanplusplus
Copy link
Member

I think tat for a novice, seeing a stack trace for the first time it is not so easy to figure what the tool is trying to tell you, never mind what it is they need to do about it. Track documentation will be key.

I can confirm that most junior professional C developers struggle to digest ASAN's output. As a learner, it's probably preferable to have a human deliver the bad news the first few times.

@ryanplusplus
Copy link
Member

Can't we just always use the AddressSanitizer?

To me, that sounds like a very good plan.

I'm pretty sure that ASAN doesn't exist yet for MacOS on ARM. Do we know if that's a large part of our userbase at this point?

@ErikSchierboom
Copy link
Member

I think tat for a novice, seeing a stack trace for the first time it is not so easy to figure what the tool is trying to tell you, never mind what it is they need to do about it. Track documentation will be key

I made my comment without knowing what the output looks like. If you think the output is hard for people to parse, could you do the memcheck in the C analyzer and make its output be easier for students to understand? That way you could also include additional links and such.

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

6 participants