Skip to content

Improve valgrind support #2718

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

Merged
markuspf merged 2 commits into from
Aug 21, 2018
Merged

Improve valgrind support #2718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
88a84aa
Add ability to add padding between Bags
ChrisJefferson Jul 3, 2018
b47d368
Extend MEMORY_CANARY support to put guards on every Bag
ChrisJefferson Jul 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 73 additions & 19 deletions src/gasman.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,23 @@ enum {
};


static inline UInt WORDS_BAG(UInt size)
// BAG_SLACK is used to define a block of empty space at the end of each
// bag, which can then be marked as "not accessible" in the memory checker
// Valgrind

enum { BAG_SLACK = 0 };

// TIGHT_WORDS_BAG defines the actual amount of space a Bag requires,
// without BAG_SLACK.
static inline UInt TIGHT_WORDS_BAG(UInt size)
{
return (size + sizeof(Bag) - 1) / sizeof(Bag);
}

static inline UInt WORDS_BAG(UInt size)
{
return TIGHT_WORDS_BAG(size) + BAG_SLACK;
}

static inline Bag *DATA(BagHeader *bag)
{
Expand Down Expand Up @@ -456,24 +468,55 @@ static inline UInt IS_BAG_BODY(void * ptr)

#include <valgrind/valgrind.h>
#include <valgrind/memcheck.h>
static Int canary_size(void)

// tell valgrind that the masterpointer, bag contents and bag header of Bag
// should all be accessible
static void CANARY_ALLOW_ACCESS_BAG(Bag bag)
{
Int bufsize = (Int)EndBags - (Int)AllocBags;
return bufsize < 4096 ? bufsize : 4096;
VALGRIND_MAKE_MEM_DEFINED(bag, sizeof(Bag));
char * ptr = (char *)PTR_BAG(bag);
Int bagLength = SIZE_BAG(bag);
VALGRIND_MAKE_MEM_DEFINED(ptr, bagLength);

BagHeader * header = BAG_HEADER(bag);
VALGRIND_MAKE_MEM_DEFINED(
header, sizeof(*header) - sizeof(header->memory_canary_padding));
}

// Reverse CANARY_ALL_ACCESS_BAG, making the masterpointer, bag contents and
// bag header all inaccessible
static void CANARY_FORBID_ACCESS_BAG(Bag bag)
{
VALGRIND_MAKE_MEM_NOACCESS(bag, sizeof(Bag));
char * ptr = (char *)PTR_BAG(bag);
Int bagLength = SIZE_BAG(bag);
VALGRIND_MAKE_MEM_NOACCESS(ptr, bagLength);

BagHeader * header = BAG_HEADER(bag);
VALGRIND_MAKE_MEM_NOACCESS(
header, sizeof(*header) - sizeof(header->memory_canary_padding));
}

static void ADD_CANARY(void)
// Mark all bags as accessible
static void CANARY_ALLOW_ACCESS_ALL_BAGS(void)
{
VALGRIND_MAKE_MEM_NOACCESS(AllocBags, canary_size());
CallbackForAllBags(CANARY_ALLOW_ACCESS_BAG);
}

static void CLEAR_CANARY(void)
// Mark all bags as inaccessible
static void CANARY_FORBID_ACCESS_ALL_BAGS(void)
{
VALGRIND_MAKE_MEM_DEFINED(AllocBags, canary_size());
VALGRIND_MAKE_MEM_NOACCESS(MptrBags, (EndBags - MptrBags) * sizeof(Bag));
}

// Temporarily disable valgrind checking. This is used while creating bags or
// adjusting any internal GASMAN structures
#define CANARY_DISABLE_VALGRIND() VALGRIND_DISABLE_ERROR_REPORTING

// Renable valgrind checking.
#define CANARY_ENABLE_VALGRIND() VALGRIND_ENABLE_ERROR_REPORTING

// CHANGED_BAG must be here to disable/enable valgrind
void CHANGED_BAG(Bag bag)
{
CANARY_DISABLE_VALGRIND();
Expand All @@ -484,10 +527,12 @@ void CHANGED_BAG(Bag bag)
CANARY_ENABLE_VALGRIND();
}
#else
#define ADD_CANARY()
#define CLEAR_CANARY()
#define CANARY_DISABLE_VALGRIND()
#define CANARY_ENABLE_VALGRIND()
#define CANARY_ALLOW_ACCESS_BAG(b)
#define CANARY_FORBID_ACCESS_BAG(b)
#define CANARY_ALLOW_ACCESS_ALL_BAGS()
#define CANARY_FORBID_ACCESS_ALL_BAGS()
#endif

/****************************************************************************
Expand Down Expand Up @@ -1200,6 +1245,7 @@ void InitBags (
ChangedBags = 0;

GAP_ASSERT(SanityCheckGasmanPointers());
CANARY_FORBID_ACCESS_ALL_BAGS();
}


Expand Down Expand Up @@ -1252,6 +1298,8 @@ Bag NewBag (
CollectBags(0,0);
#endif

CANARY_DISABLE_VALGRIND();

/* check that a masterpointer and enough storage are available */
if ( (FreeMptrBags == 0 || SizeAllocationArea < WORDS_BAG(sizeof(BagHeader)+size))
&& CollectBags( size, 0 ) == 0 )
Expand All @@ -1274,11 +1322,10 @@ Bag NewBag (
/* get the identifier of the bag and set 'FreeMptrBags' to the next */
bag = FreeMptrBags;
FreeMptrBags = *(Bag*)bag;
CLEAR_CANARY();

/* allocate the storage for the bag */
BagHeader * header = (BagHeader *)AllocBags;
AllocBags = DATA(header) + WORDS_BAG(size);
ADD_CANARY();

// enter bag header
header->type = type;
Expand All @@ -1294,8 +1341,12 @@ Bag NewBag (
/* set the masterpointer */
SET_PTR_BAG(bag, DATA(header));

CANARY_ALLOW_ACCESS_BAG(bag);

GAP_ASSERT(SanityCheckGasmanPointers());

CANARY_ENABLE_VALGRIND();

/* return the identifier of the new bag */
return bag;
}
Expand Down Expand Up @@ -1427,6 +1478,10 @@ UInt ResizeBag (
CollectBags(0,0);
#endif

CANARY_DISABLE_VALGRIND();

CANARY_FORBID_ACCESS_BAG(bag);

BagHeader * header = BAG_HEADER(bag);
UInt type = header->type;
UInt flags = header->flags;
Expand Down Expand Up @@ -1476,7 +1531,6 @@ UInt ResizeBag (

// if the last bag is enlarged ...
else if (CONST_PTR_BAG(bag) + WORDS_BAG(old_size) == AllocBags) {
CLEAR_CANARY();
// check that enough storage for the new bag is available
if (SpaceBetweenPointers(EndBags, CONST_PTR_BAG(bag)) < WORDS_BAG(new_size)
&& CollectBags( new_size-old_size, 0 ) == 0 ) {
Expand All @@ -1497,8 +1551,6 @@ UInt ResizeBag (
#endif
SizeAllBags += new_size - old_size;

ADD_CANARY();

header->size = new_size;
#if SIZEOF_VOID_P == 4
GAP_ASSERT(header->reserved == 0);
Expand All @@ -1513,23 +1565,22 @@ UInt ResizeBag (
&& CollectBags( new_size, 0 ) == 0 ) {
Panic("Cannot extend the workspace any more!!!!!!");
}
CLEAR_CANARY();

// update header pointer in case bag moved
header = BAG_HEADER(bag);

// leave magic size-type word for the sweeper, type must be T_DUMMY
header->type = T_DUMMY;
header->flags = 0;
header->size = sizeof(BagHeader) + (WORDS_BAG(old_size) - 1) * sizeof(Bag);
header->size =
sizeof(BagHeader) + (TIGHT_WORDS_BAG(old_size) - 1) * sizeof(Bag);
#if SIZEOF_VOID_P == 4
GAP_ASSERT(header->reserved == 0);
#endif

/* allocate the storage for the bag */
BagHeader * newHeader = (BagHeader *)AllocBags;
AllocBags = DATA(newHeader) + WORDS_BAG(new_size);
ADD_CANARY();

newHeader->type = type;
newHeader->flags = flags;
Expand Down Expand Up @@ -1573,6 +1624,8 @@ UInt ResizeBag (
}

GAP_ASSERT(SanityCheckGasmanPointers());
CANARY_ALLOW_ACCESS_BAG(bag);
CANARY_ENABLE_VALGRIND();
/* return success */
return 1;
}
Expand Down Expand Up @@ -1860,7 +1913,7 @@ UInt CollectBags (

GAP_ASSERT(SanityCheckGasmanPointers());
CANARY_DISABLE_VALGRIND();
CLEAR_CANARY();
CANARY_FORBID_ACCESS_ALL_BAGS();
#ifdef DEBUG_MASTERPOINTERS
CheckMasterPointers();
#endif
Expand Down Expand Up @@ -2312,6 +2365,7 @@ UInt CollectBags (
/* Possibly advise the operating system about unused pages: */
SyMAdviseFree();

CANARY_ALLOW_ACCESS_ALL_BAGS();
CANARY_ENABLE_VALGRIND();

GAP_ASSERT(SanityCheckGasmanPointers());
Expand Down
5 changes: 5 additions & 0 deletions src/gasman.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ typedef struct {
#ifdef USE_GASMAN
Bag link;
#endif
#if defined(MEMORY_CANARY)
// The following variable is marked as not readable or writable
// in valgrind, to check for code reading before the start of a Bag.
uint64_t memory_canary_padding[8];
#endif
} BagHeader;


Expand Down