The other flags in VarEvalFlags already describe their effects, not the
place where they are used. It's more important to know the effect.
Only a single unit test had to be adjusted. This probably means that
there are too few tests where the special effects of VARE_KEEP_DOLLAR
come into play. It could also mean that the effects are so simple and
obvious that they don't need any debug log, but that's not the case.
In 1993, the variable names could not refer to other variables yet.
This has been made possible on 2000-05-11, when the "cool magic" was
added that allows assigning to VAR.${param}.
The purpose of a main function is to give a high-level overview about the
whole program. 270 lines of code with lots of tricky details did not serve this
purpose. Split the code into functions corresponding to the phases.
The targets need to be copied to the 'examine' queue, not because the
targets list would be modified but because the queue is modified and the
targets list should not be affected by that.
This gets rid of a few void pointers and an unnecessary and unused
function parameter.
The variable name "bn" may have meant "before", but that was not
obvious. The new name "ogn" nicely matches the ".ORDER" in the debug
message.
This gets rid of a few void pointers and unspecific variable names like
"l" for the list that should have rather been called "examine" all the
time.
Add quotes around placeholders in debug messages. Especially for targets
like "all" the message had been syntactically misleading.
This gets rid of a few void pointers and some function calls. The
documentation of MakeFindChild essentially repeated what the code does,
and the redundant parts of it have been removed.
Before, a wrong cause for being out-of-date was printed in the debug log,
for optional cohorts. This was caused by having the same conditions
duplicated in the code, instead of putting them in a separate function.
Now the optional cohort is correctly identified as using the '::'
dependency operator.
Only some callers actually needed the updated time, and because of the
many branches, it was difficult to see that the return value was indeed
gn->mtime all the time.
It doesn't matter which of the make modules is in charge of determining
whether a node is out-of-date. Therefore, remove the module name from
the function name.
This keeps the indentation of the code small.
It also reduces the possible confusion about the two similar branches in
that function that differ in a small but important detail:
ReadAllMakefiles reads all the makefiles while ReadFirstDefaultMakefile
stops after the first existing makefile.
It's unlikely that anyone ever defines an environment variable named
".MAKE" and then runs make with the -e option, which would make the
environment variable stronger than the built-in global variable.
That's the name that is used for list nodes outside lst.c, and there is
no reason to use a different name in the implementation of lists. Sure,
the "l" of the name "ln" is redundant, but it's still shorter than
"node", and the latter sounds too similar to GNode.
The file suff.c defines both Src and Suff, which makes the variable name
s ambiguous.
The word 'target' is ambiguous as well since it could mean a GNode or the
target suffix of a transformation. Therefore, make the variable names
longer but more precise.
The comment about the nil return and the beanhead had been outdated
already in 1993. There is no nil return anywhere nearby. The longer
variable names make the rest of the comment redundant.
This code is called for each command that is parsed. Calling strstr with
4 strings that all start with the same character is unnecessary work.
Therefore, replace strstr with manually optimized code. Neither GCC
5.5.0 nor GCC 10 inlines strncmp like this, otherwise I would have used
that.
Change in behavior: previously, a${MAKE}b would not be considered to be a
sub-make command, which is probably correct but does not occur in
practice. The check for non-alphanumeric characters around the found
string was probably meant only for the plain word "make".
The generated code stays exactly the same. The only changes will be the
line numbers of assertions. To preserve them, the removed lines have
been filled up with comments and will be removed in the follow-up commit.
All this dance of determining the needed escape characters before
iterating, saving them upfront, evaluating them later using complicated
boolean expressions was not necessary at all. All that is needed is a
simple NeedsEscapes, called at just the right time.
The parentheses were confusing for human readers.
The compiler doesn't really care about the wording of the condition, GCC
5 on amd64 generates non-obvious but nice code anyway, replacing the
logical or with a logical and.
These definitions have originally been added in arch.c 1.27 on
1998-05-21. Even back then they had been unused, at least they had not
been used directly. Since macros are expanded at their use site, there
could have been an indirect use, but that is not the case anymore.
It's no wonder that nobody is using the archive handling of make. The
archives created by GNU binutils cannot be processed using make since the
format of the archive names has changed. GNU binutils appends a slash to
the member names. Support that format from now on.
Add more debugging output in ArchFindMember. Since nobody uses this part
of make, it doesn't hurt that the debug output is now very verbose.
In Arch_Touch and Arch_TouchLib, move the snprintf to where it belongs.
There's no point modifying a local variable just to throw it away
afterwards.
Comparing a string to a space-padded string is complicated enough to be
extracted to a separate function.
The behavior changes a little bit. Before, when searching for an archive
member with a short name (one that is space-padded in the archive), that
member was not searched using the AR_EFMT1 archive format. This doesn't
matter in practice though since no regular archive member has a name
starting with "#1/".
* Replace character literal 0 with '\0'.
* Replace pointer literal 0 with NULL.
* Remove redundant parentheses.
* Parentheses in multi-line conditions are not redundant at the
beginning of a line.
* Replace a few !ptr with ptr == NULL.
* Replace a few ptr with ptr != NULL.
* Replace (expr & mask) == 0 with !(expr & mask).
* Remove redundant braces for blocks in cases where the generated code
stays the same. (Assertions further down in the code would get
different line numbers.)
* Rename parameters in CondParser_String to reflect the data flow.
* Replace #ifdef notdef with #if 0.
The generated code stays exactly the same, at least with GCC 5.5.0 on
NetBSD 8.0 amd64 using the default configuration.
It was too confusing to have a function named die that doesn't actually
die. Plus, the return type int didn't give any clue about what the
function actually returns.
The wrong negation had been added in main.c 1.414 from 2020-10-31.
Found by GCC 10, which complained about a potential null pointer
dereference in line 2188.
When make is compiled with -DUSE_UCHAR_BOOLEAN, these tests failed.
Merge duplicate code and don't depend on the actual value of TRUE when
evaluating conditions.
A parameter named pp is usually used as the parsing position, which is
updated upon successful return. Not so in ParseVarnameLong, where it
was updated in the unsuccessful branch only.
To avoid confusion, rename it to out_FALSE_pp, which is a longer name
but expresses more clearly what actually happens.
The only possible values for extramodifiers are "H:" and "T:", therefore
parsing is independent of startc and endc. Use '\0' instead of '(' and
')' to remove any possible confusion about how '{' and '}' would be
handled.
The error handling in variable expressions is inconsistent. Some errors
are detected, most aren't. In particular, the error message for
undefined variables is _not_ issued on undefined variables but instead
on parse errors.
Robust programs don't have extern variable declarations in .c files, as
that risks incomatible definitions that are not detected by the compiler
and invoke undefined behavior. Make make a little more robust.
The variable discardUndefined has an implicit negation in its name,
which makes it hard to understand. Plus, most of the time it is true.
It's better to have a flag that is false most of the time and has a
positive name.
On the first attempt of inverting that variable, I stumbled upon
MainParseArgs, which initially leaves discardUndefined == FALSE, and
after handling the dashed options, sets it to TRUE. This would make a
difference when more command line arguments would be added later via the
.MAKEFLAGS special target.
Upon further inspection, the only place where discardUndefined is used
is in VarAssign_EvalSubst in parse.c, and that place is not reachable
from any of the dashed options. Therefore, discardUndefined could
already be set at the very beginning of MainParseArgs or even when
initializing the global variable itself, without any observable
difference.
Not even the ::= variable modifier could do anything about this since it
is not reachable from the dashed command line options as well, and in
addition, it expands its right-hand side in any case, always discarding
undefined variables. Oh, these little inconsistencies everywhere.
Whenever varUndefined is returned from another function, that is only
done if eflags does not contain VARE_UNDEFERR. Therefore, testing for
that flag is unnecessary.
Special-casing this variable only made the code more complicated.
Furthermore, it is not related to error handling in any way and
therefore distracted the reader from this topic.
Even though the pointer was out-of-bounds, a crash was unlikely in
practice, since typical C compilers don't check the pointers for invalid
values after each modification. The memory it pointed to was not
accessed though.
This change doesn't change any of the unit tests since the error
handling code is not yet complete, see the many "handle errors" in the
code. Nevertheless, the "out_FALSE_res = VPR_PARSE_MSG" was wrong since
the error message was only printed in lint mode, not in default mode.
Whether or not a variable is a pointer is obvious from the context.
Since the introduction of function prototypes in C90, this information
is checked by the compiler and no longer needs to be encoded in the
variable names.
When the code was still in ApplyModifiers, the variable nested_p was
necessary to distinguish the parsing position in the nested modifier
from the parsing position of the main expression.
Despite its popularity and usefulness, the variable modifier :M is
implemented so weirdly that it's not surprising people get confused
about make's parsing and escaping rules.
Before, integer overflow in the :[1..2] modifier had not been detected,
and the actual behavior varied between ILP64 and LP64I32 machines.
Before, the :ts modifier accepted character literals like \012345 and
\x1F600, which don't fit in a single character and were thus truncated.
Before, the :range modifier issued an "Unknown modifier" error message
for :range=x, which was not quite correct. The error message in this
case is now "Invalid number".
Calling Parse_Error during parsing has always led to a nonzero exit
status. Calling Parse_Error later, when expanding the shell commands,
has had no effect on the exit status. Neither had calling Error.
To make make a reliable tool, it has to report errors as they occur.
Enable this strict behavior in lint mode for now. Lint mode has to be
enabled explicitly, preserving the default behavior.
By the way, the Address Sanitizer that ran over this code on 2015-11-26
didn't find the other out-of-bounds bug. Most probably the Address
Sanitizer only detected obvious bugs in the actual test data, and there
was no test case in which .MAKE.MAKEFILES was shorter than the newly
added makefile.
These variable modifiers accept an optional timestamp in seconds, to
select which date to print. This feature is only used very rarely. The
NetBSD build doesn't use it at all, and the FreeBSD build mainly uses
the plain modifiers :gmtime and :localtime, but not their optional
argument :gmtime=1500000000.
Therefore, this change is not going to affect many builds. Those that
are indeed affected had been wrong all the time anyway.
At parse time, these errors stop the build, as intended. After that,
when the actual shell commands of the targets are expanded and run,
these errors don't stop anything, the build just continues as if nothing
had happened. This is a general problem with Var_Parse, see the many
"handle errors" markers in the code. Another problem is that on parse
errors, parsing continues and spits out spurious strings of the form
"mtime" and "ocaltime". This as well is a general problem with error
handling in make.
ok sjg
Skip access check if path is curdir.
This ensures that all proper initialization is done at least once.
If path is not curdir it should be writable to be useful.
Reviewed by: rillig
The change in main.c 1.413 broke the NetBSD build.sh if it uses a
read-only source tree, as in the daily builds.
Original commit:
https://mail-index.netbsd.org/source-changes/2020/10/31/msg123560.html
Build log:
make warning: /home/source/ab/HEAD/src: Permission denied.
[1] Segmentation fault "${make}" -m ${T...
The condition for the context is the same for both short and long names,
therefore move that condition to the only caller.
Clean up the comment and move its parts to the appropriate places. The
"with the dollar sign escaped" part had been wrong already in 1993, and
it didn't get better over time.
Reorder the parameters to match the documentation comment, and the
remaining parameters in chronological order. Remove the unused
parameter ctxt. The callbacks that need it pass it in their
modifyWordArgs instead.
Since there is only a single variable left that needs to be freed at the
end (and probably never actually needs to be freed since nobody defines
an environment variable named .OBJDIR), there is no need to loop over
these variables.
rillig