Skip to content

Possible memory leak not detected #169

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
Closed
Gamecock opened this issue Sep 15, 2017 · 27 comments
Closed

Possible memory leak not detected #169

Gamecock opened this issue Sep 15, 2017 · 27 comments

Comments

@Gamecock
Copy link
Contributor

This issue was raised by byelipk in his exercise. I don't think it applies in that case, but ran again using malloc and no error, I'm not used to Unity yet, but I think there should be one for a malloc without free. Creating the issue until I can dig deeper.

Code I ran to test:

#include "raindrops.h"
#include "stdio.h"
#include "string.h"
#include <stdlib.h>

#define PLING_FACTOR 3
#define PLANG_FACTOR 5
#define PLONG_FACTOR 7

char *convert(char *buffer, int buffer_length, int drops)
{
   memset(buffer, '\0', sizeof(char) * buffer_length);
   if ((drops % PLING_FACTOR != 0) &&
       (drops % PLANG_FACTOR != 0) && (drops % PLONG_FACTOR != 0)) {
      snprintf(buffer, buffer_length, "%d", drops);
   } else {
      snprintf
          (buffer,
           buffer_length,
           "%s%s%s",
           drops % PLING_FACTOR == 0 ? "Pling" : "",
           drops % PLANG_FACTOR == 0 ? "Plang" : "",
           drops % PLONG_FACTOR == 0 ? "Plong" : "");
   }
    char* newbuffer = malloc(sizeof(char) * buffer_length);
    memcpy(newbuffer,buffer,buffer_length);
   return newbuffer;
}

Completely unrelated to the title, but raindrops_test.c needs to be updated to check the value in the buffer, not just the return value. They should match. I can address that at the same time or with a different patch. However you want.
@arcuru
Copy link
Contributor

arcuru commented Sep 15, 2017

The Unity test suite doesn't check for memory leaks, so there shouldn't be any warning about that.

In this repo we run the tests twice, the second time using valgrind to check for memory leaks. But that only has an effect on code checked into this repo and not on the user's solutions.

@ryanplusplus
Copy link
Member

Right, Unity doesn't support this. CppUTest does support it, but breaks frequently enough with updated toolchains that I would rather not go this route.

@patricksjackson what do you think about possibly making valgrind an optional install that our makefiles can use if it is available on the user's system?

@arcuru
Copy link
Contributor

arcuru commented Sep 16, 2017

I think that's a pretty good idea :) Were you thinking about adding it as a separate target in the makefile or adding it to make test if it's on the system?

Personally, I think a separate make target might be better. We could document it to the user in the same way that some of the problems have 'Extra Credit' tests.

@ryanplusplus
Copy link
Member

I was actually thinking of having it run all the time if it was detected on the system. Is there any downside to having it always run? I have used valgrind very little, so I'm not sure what impact it would make.

@arcuru
Copy link
Contributor

arcuru commented Sep 16, 2017

It does slow things down, but for these small tests that should have very few heap allocations it shouldn't slow them down by a lot, maybe 2-3x at the high end.

The output can get very chatty and I'd guess is extremely confusing for someone new to a systems language (we could just call it pass/fail like in the CI script, but then there's no way to debug AFAIK). It's also possible that even if valgrind is installed some people would want to try to solve the tests first and then try to do the 'Extra Credit' of fixing the memory leaks, or just not bother trying to solve it that way for a particular problem.

@ryanplusplus
Copy link
Member

Makes sense. I'm on board with a second make target.

@Gamecock Gamecock mentioned this issue Oct 3, 2017
Closed
@arcuru
Copy link
Contributor

arcuru commented Oct 7, 2017

I'm adding some tags here and describing what this is looking for:

Basically, this thread is saying it would be good to add a new make target to run the tests under Valgrind. With extra credit if you can have it pretty print whether or not there were errors, and re-use the target in bin/run-tests

A minimal case:

memcheck: tests.out
    @echo Running memory test
    valgrind --tool=memcheck ./tests.out

@Gamecock
Copy link
Contributor Author

I ran valgrind using the parameters in bin/run-tests and it found a possible memeory leak in hello world, but it does not really point me anywhere. Is this the expected output, and do we think it's useful as a generic output from an exercise?

$ valgrind --quiet --leak-check=full ./tests.out
==15340== Syscall param msg->desc.port.name points to uninitialised byte(s)
==15340==    at 0x1003AA34A: mach_msg_trap (in /usr/lib/system/libsystem_kernel.dylib)
==15340==    by 0x1003A9796: mach_msg (in /usr/lib/system/libsystem_kernel.dylib)
==15340==    by 0x1003A3485: task_set_special_port (in /usr/lib/system/libsystem_kernel.dylib)
==15340==    by 0x10053F10E: _os_trace_create_debug_control_port (in /usr/lib/system/libsystem_trace.dylib)
==15340==    by 0x10053F458: _libtrace_init (in /usr/lib/system/libsystem_trace.dylib)
==15340==    by 0x1000A89DF: libSystem_initializer (in /usr/lib/libSystem.B.dylib)
==15340==    by 0x10001AA1A: ImageLoaderMachO::doModInitFunctions(ImageLoader::LinkContext const&) (in /usr/lib/dyld)
==15340==    by 0x10001AC1D: ImageLoaderMachO::doInitialization(ImageLoader::LinkContext const&) (in /usr/lib/dyld)
==15340==    by 0x1000164A9: ImageLoader::recursiveInitialization(ImageLoader::LinkContext const&, unsigned int, char const*, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) (in /usr/lib/dyld)
==15340==    by 0x100016440: ImageLoader::recursiveInitialization(ImageLoader::LinkContext const&, unsigned int, char const*, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) (in /usr/lib/dyld)
==15340==    by 0x100015523: ImageLoader::processInitializers(ImageLoader::LinkContext const&, unsigned int, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) (in /usr/lib/dyld)
==15340==    by 0x1000155B8: ImageLoader::runInitializers(ImageLoader::LinkContext const&, ImageLoader::InitializerTimingList&) (in /usr/lib/dyld)
==15340==  Address 0x10488d76c is on thread 1's stack
==15340==  in frame #2, created by task_set_special_port (???:)
==15340== 
test/test_hello_world.c:22:test_hello:PASS

-----------------------
1 Tests 0 Failures 0 Ignored 
OK
==15340== 72 bytes in 3 blocks are possibly lost in loss record 26 of 43
==15340==    at 0x10009AC7A: calloc (in /usr/local/Cellar/valgrind/3.13.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==15340==    by 0x1005B3846: map_images_nolock (in /usr/lib/libobjc.A.dylib)
==15340==    by 0x1005C6FE8: objc_object::sidetable_retainCount() (in /usr/lib/libobjc.A.dylib)
==15340==    by 0x10000A03B: dyld::notifyBatchPartial(dyld_image_states, bool, char const* (*)(dyld_image_states, unsigned int, dyld_image_info const*), bool, bool) (in /usr/lib/dyld)
==15340==    by 0x10000A255: dyld::registerObjCNotifiers(void (*)(unsigned int, char const* const*, mach_header const* const*), void (*)(char const*, mach_header const*), void (*)(char const*, mach_header const*)) (in /usr/lib/dyld)
==15340==    by 0x10020100A: _dyld_objc_notify_register (in /usr/lib/system/libdyld.dylib)
==15340==    by 0x1005B3074: _objc_init (in /usr/lib/libobjc.A.dylib)
==15340==    by 0x10019468D: _os_object_init (in /usr/lib/system/libdispatch.dylib)
==15340==    by 0x10019463A: libdispatch_init (in /usr/lib/system/libdispatch.dylib)
==15340==    by 0x1000A89D5: libSystem_initializer (in /usr/lib/libSystem.B.dylib)
==15340==    by 0x10001AA1A: ImageLoaderMachO::doModInitFunctions(ImageLoader::LinkContext const&) (in /usr/lib/dyld)
==15340==    by 0x10001AC1D: ImageLoaderMachO::doInitialization(ImageLoader::LinkContext const&) (in /usr/lib/dyld)
==15340== 
$```

@arcuru
Copy link
Contributor

arcuru commented Oct 20, 2017

Looks like you're on a Mac, so it's probably this -> https://stackoverflow.com/questions/5226691/valgrind-mac-os-mem-leak

First answer has a supposed fix. Valgrind without errors is relatively easy to read, but you may want to run it without the --quiet option for testing.

@Gamecock Gamecock mentioned this issue Nov 6, 2017
Open
@wolf99
Copy link
Contributor

wolf99 commented Nov 9, 2017

To get this moving again I had a little play around with the makefile for the hello-world exercise.

To begin with I note above that MacOS builds require a suppression file. Following the link Patrick gave I downloaded the latest valgrind and added darwin16.supp from it to the ./config/ directory and then referenced that in the build command (below).

I've not used valgrind suppressions before, is this the correct way to refer to this file, or is it available as part of the valgrind install somewhere? If correct, is ./config a suitable location for it?

Furthermore I don't have access to test on the Mac platform, if @Gamecock could test if this A)suppresses irrelevant information B) does not suppress relevant information on MacOS, it would be great help 👍

I adjusted the makefile like so, note the additional -g and explicit -O0 flags for the build of tests.out affects the regular test make target.

CFLAGS  = -std=c99
CFLAGS += -Wall
CFLAGS += -Wextra
CFLAGS += -pedantic
CFLAGS += -Werror
CFLAGS += -O0
CFLAGS += -g

VFLAGS  = --quiet
VFLAGS += --suppressions=../../config/darwin16.supp
VFLAGS += --tool=memcheck
VFLAGS += --leak-check=full
VFLAGS += --error-exitcode=1

test: tests.out
    @./tests.out

memcheck: tests.out
    @valgrind $(VFLAGS) ./tests.out >> /dev/null

clean:
    rm -f *.o *.out

tests.out: test/test_hello_world.c src/hello_world.c src/hello_world.h
    @echo Compiling $@
    @cc $(CFLAGS) src/hello_world.c test/vendor/unity.c test/test_hello_world.c -o tests.out

This seems to work for all the playing about that I did with it with the hello-world example. So if nobody has further suggestions or adjustments we can go ahead and start updating each exercise's respective makefile.

@Gamecock
Copy link
Contributor Author

Gamecock commented Nov 9, 2017

Where did you find the darwin16.supp, I looked here and it only has as high as darwin14.
I tried using it, and got the same gibberish.

Also, don't think config is the right place for suppression file, since students won't get it.needs to be added to something in the exercise folder. Maybe in vendor so the student just ignores?

@wolf99
Copy link
Contributor

wolf99 commented Nov 9, 2017 •
edited
Loading

The suppression file is in the valgrind source distribution from the valgrind.org website. I realised the svn2github mirror mentioned in the linked SO question hasn't seen updates in quite some time and that the suppressions don't cover newer versions of MacOS. So I checked to see if there was a more recent version at http://valgrind.org/downloads/current.html 😃

WRT placing the suppression file in each exercise, I had forgotten the students would need access to it as part of the exercise! The vendor file seems logical from the current structure. This does have the downside of needing to change every exercise whenever updating to a newer version though - not sure if that is a concern we need to cover in this case?

@Gamecock
Copy link
Contributor Author

Gamecock commented Nov 9, 2017

OK, digging into this a little more I have I have darwin16.supp installed by default, so that error above cannot be fixed with the supression, but I would guess we only have to add the --suppressions flag if this is a leak unity or our test, in which case we coud just go fix.
@ryanplusplus can you see if you get the same error on your mac. I am running darwin 16.7.0, it might be something I installed, or maybe I need to submit a patch to valgrind.

@wolf99
Copy link
Contributor

wolf99 commented Nov 9, 2017 •
edited
Loading

As regards my earlier question of if this is the correct way of obtaining this file; it seems that pre-built valgrind packages only have the supp file specific to the platform for which they are built (via the default.supp), the others are not directly available with that build - thus, yes by default, platform-specific .supp files must be obtained from the source repository (if they are to be included in the exercise directory).

@Gamecock Are you saying that you tried on MacOS, with suppressions, yet still saw those errors?

@Gamecock
Copy link
Contributor Author

Yes,
I have 1 uninitialized bytes and 1 possible leak with the darwin16.supp installed. Either the darwin.supp needs to be updated, or I have something else corrupting my machine.

If you install valgrind with brew the darwin16.supp is installed as part of your default.

@wolf99
Copy link
Contributor

wolf99 commented Nov 10, 2017

There's an article on generating suppressions here: http://kalapun.com/posts/checking-c-code-with-valgrind-on-yosemite/

It might be interesting to see what that spits out for your system @Gamecock

However I note that, with the small bit of research I have done this morning, that there has been problems with valgrind on Mac, on and off, for years so it may not be something we can easily solve here.

@Gamecock
Copy link
Contributor Author

Gamecock commented Nov 10, 2017 via email

@wolf99
Copy link
Contributor

wolf99 commented Nov 10, 2017

I may have access to a Mac this evening to do some playing about on, but so far your suggestion to leave suppressions up to the user seems like the most sustainable @Gamecock. Seeing as a memory check build is not vital to, nor required for, considering an exercise completed, I'd say that it is a very viable approach too.

@wolf99
Copy link
Contributor

wolf99 commented Nov 10, 2017

Unfortunately I could not even get valgrind installed on the Mac!

Anyway, I say for the sake of ease of maintenance etc, we go with gamecock's idea of not bothering with suppressions for now. I'll raise a PR at some stage this evening for that.

@Gamecock
Copy link
Contributor Author

Gamecock commented Nov 10, 2017 via email

@wolf99
Copy link
Contributor

wolf99 commented Nov 10, 2017

Both. Can't immediately recall why brew didn't work out but it seems that the gcc that was on the machine wasn't suitable for building valgrind according to what configure.sh reported. I did try to figure out a way around it but A) my time on the machine was limited (wasn't my rig) and I was wary of going down the rabbit hole of installing a never ending list of things just to fix the last thing 😅 , B) I'm so unused to macs (I'm a life-long windows user and very occasional Linux dabbler) that every new problem makes things exponentially harder.

I guess if we want to fix the Mac support that could be a future issue/PR? Having Just Windows and Linux support for now is better than the current no-support 😄 .

@Gamecock
Copy link
Contributor Author

Gamecock commented Nov 10, 2017 via email

@wolf99 wolf99 mentioned this issue Nov 10, 2017
Merged
@wolf99
Copy link
Contributor

wolf99 commented Nov 10, 2017 •
edited
Loading

OK, then. If everyone is OK with adding as in #241 I guess we can go ahead and start raising similar for each exercise.

@wolf99
Copy link
Contributor

wolf99 commented Nov 15, 2017

#241 has been merged with the following additions to the makefile for hello-world

CFLAGS += -g

VFLAGS  = --quiet
VFLAGS += --tool=memcheck
VFLAGS += --leak-check=full
VFLAGS += --error-exitcode=1

memcheck: tests.out
	@valgrind $(VFLAGS) ./tests.out
	@echo "Memory check passed"

This then needs to be applied to the makefile for every other exercise.

As commented in that PR, the run-tests could then be updated to use the memcheck target instead of calling both the test target and valgrind.

@wolf99 wolf99 mentioned this issue Nov 15, 2017
Merged
@Gamecock
Copy link
Contributor Author

@wolf99 do you want help with any other exercises or do you got this. Don't want to duplicate any work.

@wolf99
Copy link
Contributor

wolf99 commented Nov 15, 2017 •
edited
Loading

Thanks, I can work through them in #224 though, it's not much work per exercise. Will need eyes on as I go though, to review for any silly mistakes 🤣 . I plan to do a commit per exercise.

@wolf99
Copy link
Contributor

wolf99 commented Nov 22, 2017

Closed by #244

@wolf99 wolf99 closed this as completed Nov 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@arcuru @wolf99 @ryanplusplus @Gamecock