Implement special handling for MAKE macro

[illiliti]

See also: https://www.austingroupbugs.net/view.php?id=1155

---

TODO:

• [x] tests
• [x] docs/readme

[illiliti]

I found a bug that i don't know how to fix. Consider the following example:

printf 'MAKE=bye\n\na:\n\t@echo $(MAKE)\n' | pdpmake -f - MAKE=hello

bmake, gmake, smake prints hello here, while pdpmake prints bye. Any idea how to fix?

[illiliti]

By the way, I noticed that bmake is quite different from other implementations while working on this.

Example 1:

printf 'a:\n\techo $(MAKE)\n' | MAKE=hello bmake -f -

• gmake, smake, pdpmake: hello
• bmake: bmake

Example 2:

printf 'a:\n\techo $(MAKE)\n' | MAKE=hello ../../../bin/bmake -f -

• gmake, smake, pdpmake: <current working directory>/../../../bin/bmake
• bmake: /usr/bin/bmake

[rmyorston]

I think the fix is to update the stored level when the value of a macro is replaced:

diff --git a/macro.c b/macro.c
index 066de9c..1d155e0 100644
--- a/macro.c
+++ b/macro.c
@@ -51,11 +51,11 @@ setmacro(const char *name, const char *val, int level)
        macrohead[bucket] = mp;
        mp->m_flag = FALSE;
        mp->m_name = xstrdup(name);
-       mp->m_level = level;
    }
 #if ENABLE_FEATURE_MAKE_EXTENSIONS
    mp->m_simple = simple;
 #endif
+   mp->m_level = level;
    mp->m_val = xstrdup(val ? val : "");
 }

Example 1 seems to show bmake doesn't respect the (revised) standard.

Example 2 is OK. The (revised) standard says:

If this value contains at least one <slash> and is a relative pathname, make shall convert it to an absolute pathname.

It doesn't specify how the absolute pathname is to be derived. bmake has chosen to call realpath(3) to get a canonicalized path.

[illiliti]

Applied the fix, thanks!

[E5ten]

it's worth noting that realpath() is XSI, not POSIX, so the feature macro defined in make.h should be changed to _XOPEN_SOURCE. Alternatively, if there is a solution that only involves POSIX functions, that might be considered preferable depending on what @rmyorston prioritizes.

[illiliti]

feature macro defined in make.h should be changed to _XOPEN_SOURCE

Did that already

[E5ten]

feature macro defined in make.h should be changed to _XOPEN_SOURCE

Did that already

Sorry, missed that

[rmyorston]

Several comments.

First, _XOPEN_SOURCE isn't a problem. It gets the job done.

Second, the meaning of ENABLE_FEATURE_CLEANUP. I stole this from BusyBox. If it's non-zero it'll clean up resources which are still in scope at the end of a program but which it isn't necessary to clean up because they'll vanish anyway. (They just love avoiding unnecessary code in BusyBox.)

It shouldn't be used in this code:

    if (argv[0][0] != '/' && strchr(argv[0], '/')) {
        char *path = realpath(argv[0], NULL);
        if (!path) {
            error("can't resolve path for %s: %s", argv[0], strerror(errno));
        }
        setmacro("MAKE", path, 4);
#if ENABLE_FEATURE_CLEAN_UP
        free(path);
#endif
    }

because path goes out of scope at the end of the block, resulting in a leak if ENABLE_FEATURE_CLEAN_UP is zero. path should be freed unconditionally in this case. However, see later.

Third, the fix in setmacro() is required because the call to setmacro("MAKE", path, 4) is out of order. Macros are (usually) set with increasing values of level (the third argument) to track their source and implement the rule that:

Macro definitions from these sources shall not override macro definitions from a lower-numbered source.

The SHELL macro is also set out of order, but it gets away with it because there are additional special rules that apply.

It seems sensible to move the setting of MAKE and SHELL down to the correct position in the code. Doing this avoids the need for the fix in setmacro() but it's probably wise to leave it there to catch any similar cases that might turn up in future.

What we can't do is move the creation of path because that has to happen before any -C options. As a result path is now in scope throughout main() and can be cleaned up conditionally.

These changes, and some other random shuffling, are in this revised version of the first patch: 0001-Implement-special-handling-for-MAKE-macro.txt. Only main.c is affected.

[illiliti]

Patch applied, thanks!

(Side note: you can (force-)push patches to my branch directly if you like)

resulting in a leak if ENABLE_FEATURE_CLEAN_UP is zero.

I still don't understand. This scope thing doesn't have any sense. My question is, are intentional leaks allowed? If not, why there's ENABLED_FEATURE_CLEAN_UP? If yes, why do i have to free? I don't understand this "middle ground", when some things being free'd and some don't.

[rmyorston]

you can (force-)push patches to my branch directly if you like

Sure, I prefer not to do that.

are intentional leaks allowed?

Allocated memory that's still reachable at the end of the program run is considered to be OK.

When compiled with #define ENABLE_FEATURE_CLEANUP 1 and run under Valgrind to build itself, like so:

valgrind ./make clean make

Valgrind reports:

==2973== HEAP SUMMARY:
==2973==     in use at exit: 0 bytes in 0 blocks
==2973==   total heap usage: 1,016 allocs, 1,016 frees, 40,455 bytes allocated
==2973== 
==2973== All heap blocks were freed -- no leaks are possible

When compiled with #define ENABLE_FEATURE_CLEANUP 0 we get:

==2849== HEAP SUMMARY:
==2849==     in use at exit: 12,085 bytes in 460 blocks
==2849==   total heap usage: 1,019 allocs, 559 frees, 40,510 bytes allocated
==2849== 
==2849== LEAK SUMMARY:
==2849==    definitely lost: 0 bytes in 0 blocks
==2849==    indirectly lost: 0 bytes in 0 blocks
==2849==      possibly lost: 0 bytes in 0 blocks
==2849==    still reachable: 12,085 bytes in 460 blocks
==2849==         suppressed: 0 bytes in 0 blocks

So no memory has been lost. Everything that was allocated has either been freed or is still accessible.

[rmyorston]

OK, there are several problems with that latest revision. All my fault. Working on it.

[rmyorston]

Thing I got wrong:

• Memory allocations that are only referenced via local variables in main() are considered lost by Valgrind. So fargv0 and path need to be freed unconditionally.
• There was a typo in the clean up conditional: ENABLE_FEATURE_CLEANUP should have been ENABLE_FEATURE_CLEAN_UP. (Though, of course, it's gone now anyway.)
• We need to take a copy of argv[0] to check if path needs to be freed: at the time of the test argv has changed.

All fixed now and pulled into my repo.

Thanks for the PR and for putting up with my incompetence.

[illiliti]

Good work! Thanks for fixes