Module Name: src
Committed By: rillig
Date: Sun Nov 29 18:49:36 UTC 2020
Modified Files:
src/usr.bin/make: dir.c
src/usr.bin/make/unit-tests: Makefile
Log Message:
make(1): clean up memory management for CachedDirs
Previously, the reference count for a newly created CacheDir had been
set to 1 in CacheNewDir. This was wrong because at that point, the
object had not been referenced by any nonlocal variable. The reference
count is no longer incremented at this point.
All callers of CacheNewDir either append the newly created CachedDir to
a SearchPath via Lst_Append and CachedDir_Ref, or they assign it to a
global variable via CachedDir_Assign.
Since the reference count is no longer wrongly incremented, it does not
need to be decremented more than necessary in Dir_End. To keep the code
simple and maintainable, all assignments to global variables are now
handled by CachedDir_Assign. Adding a CachedDir to a list is still done
manually via Lst_Append, and the corresponding code for decrementing is
in SearchPath_Clean and SearchPath_Free. These details may be cleaned
up in a follow-up commit.
As a result, when OpenDirs_Done is called in the unit tests, the list of
open directories is empty. It had been non-empty in a single unit test
before (dep-wildcards.mk), as a result of calling Dir_Expand.
The additional debug logging for the reference counting is not enabled
by default since it contains memory addresses, which makes the output
dependent on the memory allocator.
The function CachedDir_Destroy has been merged into CachedDir_Undef,
which had only been used in Dir_End before. The new name emphasizes
that it corresponds to CachedDir_Ref.
To generate a diff of this commit:
cvs rdiff -u -r1.240 -r1.241 src/usr.bin/make/dir.c
cvs rdiff -u -r1.228 -r1.229 src/usr.bin/make/unit-tests/Makefile
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/usr.bin/make/dir.c
diff -u src/usr.bin/make/dir.c:1.240 src/usr.bin/make/dir.c:1.241
--- src/usr.bin/make/dir.c:1.240 Sun Nov 29 16:37:10 2020
+++ src/usr.bin/make/dir.c Sun Nov 29 18:49:36 2020
@@ -1,4 +1,4 @@
-/* $NetBSD: dir.c,v 1.240 2020/11/29 16:37:10 rillig Exp $ */
+/* $NetBSD: dir.c,v 1.241 2020/11/29 18:49:36 rillig Exp $ */
/*
* Copyright (c) 1988, 1989, 1990 The Regents of the University of California.
@@ -136,7 +136,7 @@
#include "job.h"
/* "@(#)dir.c 8.2 (Berkeley) 1/2/94" */
-MAKE_RCSID("$NetBSD: dir.c,v 1.240 2020/11/29 16:37:10 rillig Exp $");
+MAKE_RCSID("$NetBSD: dir.c,v 1.241 2020/11/29 18:49:36 rillig Exp $");
#define DIR_DEBUG0(text) DEBUG0(DIR, text)
#define DIR_DEBUG1(fmt, arg1) DEBUG1(DIR, fmt, arg1)
@@ -299,6 +299,10 @@ CachedDir_New(const char *name)
dir->hits = 0;
HashSet_Init(&dir->files);
+#ifdef DEBUG_REFCNT
+ DEBUG2(DIR, "CachedDir %p new for \"%s\"\n", dir, dir->name);
+#endif
+
return dir;
}
@@ -306,15 +310,31 @@ static CachedDir *
CachedDir_Ref(CachedDir *dir)
{
dir->refCount++;
- DEBUG2(DIR, "CachedDir refCount++ to %d for \"%s\"\n",
- dir->refCount, dir->name);
+
+#ifdef DEBUG_REFCNT
+ DEBUG3(DIR, "CachedDir %p ++ %d for \"%s\"\n",
+ dir, dir->refCount, dir->name);
+#endif
+
return dir;
}
-/* Free a cached directory with reference count 0. */
static void
-CachedDir_Free0(CachedDir *dir)
+CachedDir_Unref(CachedDir *dir)
{
+ dir->refCount--;
+
+#ifdef DEBUG_REFCNT
+ DEBUG3(DIR, "CachedDir %p -- %d for \"%s\"\n",
+ dir, dir->refCount, dir->name);
+#endif
+
+ if (dir->refCount > 0)
+ return;
+
+#ifdef DEBUG_REFCNT
+ DEBUG2(DIR, "CachedDir %p free for \"%s\"\n", dir, dir->name);
+#endif
OpenDirs_Remove(&openDirs, dir->name);
@@ -323,24 +343,6 @@ CachedDir_Free0(CachedDir *dir)
free(dir);
}
-static void
-CachedDir_Unref(CachedDir *dir)
-{
- dir->refCount--;
- DEBUG2(DIR, "CachedDir refCount-- to %d for \"%s\"\n",
- dir->refCount, dir->name);
-}
-
-/* Nuke a directory descriptor, if it is no longer used. */
-static void
-CachedDir_Destroy(CachedDir *dir)
-{
- CachedDir_Unref(dir);
-
- if (dir->refCount == 0)
- CachedDir_Free0(dir);
-}
-
/* Update the value of the CachedDir variable, updating the reference counts. */
static void
CachedDir_Assign(CachedDir **var, CachedDir *dir)
@@ -352,7 +354,7 @@ CachedDir_Assign(CachedDir **var, Cached
if (dir != NULL)
CachedDir_Ref(dir);
if (prev != NULL)
- CachedDir_Destroy(prev);
+ CachedDir_Unref(prev);
}
static void
@@ -374,7 +376,7 @@ OpenDirs_Done(OpenDirs *odirs)
CachedDir *dir = ln->datum;
DIR_DEBUG2("OpenDirs_Done: refCount %d for \"%s\"\n",
dir->refCount, dir->name);
- CachedDir_Destroy(dir); /* removes the dir from odirs->list */
+ CachedDir_Unref(dir); /* removes the dir from odirs->list */
ln = next;
}
Lst_Done(&odirs->list);
@@ -502,11 +504,7 @@ Dir_InitCur(const char *cdname)
if (dir == NULL)
return;
- if (cur != NULL && cur != dir) {
- CachedDir_Unref(cur); /* XXX: why unref twice? */
- CachedDir_Destroy(cur);
- }
- cur = CachedDir_Ref(dir);
+ CachedDir_Assign(&cur, dir);
}
/* (Re)initialize "dot" (current/object directory) path hash.
@@ -514,31 +512,16 @@ Dir_InitCur(const char *cdname)
void
Dir_InitDot(void)
{
- if (dot != NULL) {
- /* Remove old entry from openDirs, but do not destroy. */
- /* XXX: Why not destroy? It's reference-counted after all. */
- OpenDirs_Remove(&openDirs, dot->name);
- }
-
- /* XXX: Before assigning to the global variable, refCount++. */
- dot = Dir_AddDir(NULL, ".");
+ CachedDir *dir;
- if (dot == NULL) {
+ dir = Dir_AddDir(NULL, ".");
+ if (dir == NULL) {
Error("Cannot open `.' (%s)", strerror(errno));
exit(1);
}
- /*
- * We always need to have dot around, so we increment its reference
- * count to make sure it's not destroyed.
- */
- /*
- * XXX: This is just the normal reference counting. Why is the above
- * comment so long? And why doesn't the normal reference counting
- * suffice? This sounds like someone misunderstood reference counting
- * here.
- */
- CachedDir_Ref(dot);
+ CachedDir_Assign(&dot, dir);
+
Dir_SetPATH(); /* initialize */
}
@@ -547,13 +530,9 @@ void
Dir_End(void)
{
#ifdef CLEANUP
- if (cur != NULL) {
- CachedDir_Unref(cur); /* XXX: why unref twice? */
- CachedDir_Destroy(cur);
- }
+ CachedDir_Assign(&cur, NULL);
+ CachedDir_Assign(&dot, NULL);
CachedDir_Assign(&dotLast, NULL);
- CachedDir_Unref(dot); /* XXX: why unref twice? */
- CachedDir_Destroy(dot);
SearchPath_Clear(&dirSearchPath);
OpenDirs_Done(&openDirs);
HashTable_Done(&mtimes);
@@ -953,9 +932,7 @@ Dir_Expand(const char *word, SearchPath
partPath = SearchPath_New();
(void)Dir_AddDir(partPath, dirpath);
DirExpandPath(cp + 1, partPath, expansions);
- Lst_Free(partPath);
- /* XXX: Should the dirs in partPath be freed here?
- * It's not obvious whether to free them or not. */
+ SearchPath_Free(partPath);
}
}
@@ -1514,7 +1491,6 @@ CacheNewDir(const char *name, SearchPath
DIR_DEBUG1("Caching %s ...\n", name);
dir = CachedDir_New(name);
- CachedDir_Ref(dir); /* XXX: why here already? */
while ((dp = readdir(d)) != NULL) {
@@ -1534,7 +1510,7 @@ CacheNewDir(const char *name, SearchPath
OpenDirs_Add(&openDirs, dir);
if (path != NULL)
- Lst_Append(path, dir);
+ Lst_Append(path, CachedDir_Ref(dir));
DIR_DEBUG1("Caching %s done\n", name);
return dir;
@@ -1565,8 +1541,7 @@ Dir_AddDir(SearchPath *path, const char
return pathDir;
}
- CachedDir_Ref(dotLast);
- Lst_Prepend(path, dotLast);
+ Lst_Prepend(path, CachedDir_Ref(dotLast));
}
if (path != NULL) {
@@ -1644,7 +1619,7 @@ SearchPath_Free(SearchPath *path)
for (ln = path->first; ln != NULL; ln = ln->next) {
CachedDir *dir = ln->datum;
- CachedDir_Destroy(dir);
+ CachedDir_Unref(dir);
}
Lst_Free(path);
}
@@ -1656,7 +1631,7 @@ SearchPath_Clear(SearchPath *path)
{
while (!Lst_IsEmpty(path)) {
CachedDir *dir = Lst_Dequeue(path);
- CachedDir_Destroy(dir);
+ CachedDir_Unref(dir);
}
}
Index: src/usr.bin/make/unit-tests/Makefile
diff -u src/usr.bin/make/unit-tests/Makefile:1.228 src/usr.bin/make/unit-tests/Makefile:1.229
--- src/usr.bin/make/unit-tests/Makefile:1.228 Sun Nov 29 14:29:19 2020
+++ src/usr.bin/make/unit-tests/Makefile Sun Nov 29 18:49:36 2020
@@ -1,4 +1,4 @@
-# $NetBSD: Makefile,v 1.228 2020/11/29 14:29:19 rillig Exp $
+# $NetBSD: Makefile,v 1.229 2020/11/29 18:49:36 rillig Exp $
#
# Unit tests for make(1)
#
@@ -440,7 +440,7 @@ FLAGS.varname-empty= -dv '$${:U}=cmdline
# Some tests need extra postprocessing.
SED_CMDS.dir= ${:D remove output from -DCLEANUP mode }
SED_CMDS.dir+= -e '/^OpenDirs_Done:/d'
-SED_CMDS.dir+= -e '/^CachedDir refCount/d'
+SED_CMDS.dir+= -e '/^CachedDir /d'
SED_CMDS.export= -e '/^[^=_A-Za-z0-9]*=/d'
SED_CMDS.export-all= ${SED_CMDS.export}
SED_CMDS.export-env= ${SED_CMDS.export}
