@facchinm facchinm left a comment

The comments state that if ExpectFile() is called, but there are no
remaining items in the cache, it will invalidate the cache. However, the
code would only invalidate the cache if at least one item was still
present, but it didn't match the expected file. In practice, this
wouldn't usually cause issues, since adding a new file would usually
cause an invalid cache earlier on, and even if a new file was added at
the end of the compilation, it would not be in the .d file, so it would
be marked as "changed".
However, in rare circumstances, such as when the include cache would not
be properly generated due to other problems (see arduino#230), this would cause
a crash, when ExpectFile did not invalidate the cache and the file in
question was unchanged, causing an out-of-bounds read from the cache.
This commit fixes this by making ExpectFile behave like documented and
invalidate the cache when there are no remaining entries.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Previously, the relative filename passed to GCCPreprocRunner() was made
absolute by prepareGCCPreprocRecipeProperties() and then returned, so it
could be used later on. There was an exception to this when /dev/null
was passed.
Now, the only place that passed something other than /dev/null simply
does this processing up front instead. This prepares the way for
removing Context::FileToRead in the next commit.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Previously, this filename was set by GCCPreprocRunner into the context,
because the full filename was not known until then. Since the previous
commit, this filename is known by the ContainerAddPrototypes, which can
just pass it to ReadFileAndStoreInContext explicitly. This allows
Context::FileToRead to be removed.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

This pass was unused.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Previously, the verbose, logger and sometimes warningFlags were
extracted from the Context by the top-level runner and passed down
separately. Since this causes a lot of variable-passing of what is
essentially global state, it is clearer to just pass a types.Context
down and let the helpers get the data they need from that state.
This prepared for a next commit where ExecRecipe will be refactored and
needs access to the Context. Since the next commit will heavily change
ExecRecipe anyway, this commit does not actually change ExecRecipe to
accept the Context.
This commit should not change any behaviour.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

For some reason the sizer commandline was never shown. For consistency,
it is now shown in verbose mode, just like the other commands.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

This also happens with the normal compilation commands, so why not with
these? Normally these commands should not output to stdout, so this
doesn't make any difference, but it makes things more consistent.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Previously, this function would ignore any errors returned by `Run()`
since the command is expected to fail in most cases. However, in
addition to ignoring a non-zero exit code from the command, it would
also ignore errors in running the command itself.
With this commit, `ExecRecipeCollectStdErr()` simply returns all errors,
but its caller checks the type of the error. If it is `ExitError`, this
indicates a non-zero exit status, which is ignored. Otherwise, the error
is reported as normal.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Previously, the return value was converted to string. Letting callers
convert to string makes it easier to write the sterr contents to
os.Stderr again in a future commit.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

For include detection, the preprocessor is run on all source files,
collecting #included filenames from the stderr output, each of which are
then resolved to a library to include. A caching mechanism is used to
only run the preprocessor when needed.
This commit improves the error handling during include detection in a
number of ways:
 - When the preprocessor runs succesfully, processing stops for the
 current file. Previously, it would always look at stderr to find a
 missing include filename and only stop if none was found.
 - When the preprocessor fails, but no filename can be found, show the
 error preprocessor error. Previously, it would assume that the
 process was done and stop processing the file without any error.
 - When no library can be found for a missing include, show the stored
 error output instead of running the preprocessor again. Previously,
 the preprocessor would be run a second time, to (re)generate the
 error message.
 When the include filename comes from the cache and the preprocessor
 was not run yet, it is still run to show its errors to the user. This
 should be very rare, as normally changes that cause a cached filename
 to become unresolvable to a library also cause the file to be marked
 as changed, bypassing the cache. When this does happen, the
 preprocessor is now run using `GCCPreprocRunnerForDiscoveringIncludes()`
 instead of `GCCPreprocRunner()`, which ensures the preprocessor
 command is always exactly the same.
 Before this change, there could be specific circumstances where the
 first preprocessor run would generate an error, but where the second
 run would not show the error and include detection would continue as
 if nothing happened. One such circumstance is described in arduino#230.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

This unifies these similar methods into a single method. The interface
is additionally changed to:
 - Accepts a `Context` argument.
 - Allow for defaults and named arguments, using an `ExecOptions` struct
 that is passed as an argument.
 - Allow configuring command output handling in a flexible way. Instead
 of passing bools for some specific configurations, you can now pass
 either `Ignore`, `Show` or `Capture` for both stdout and stderr
 independently.
 By default, stdout is is shown when verbose is true, or ignored when
 verbose is false. Stderr is shown by default.
 - Actually redirect stdout to `/dev/null` when it is not needed (by
 leaving `command.Stdout` at nil). Previously, `ExecRecipe` would
 either show or capture stdout, and the captured output was usually
 just thrown away.
To allow for even more reuse, the biggest part of `ExecRecipe` is
extracted into a new `utils.ExecCommand()` function which executes an
arbitrary `exec.Cmd` object, with configurable output redirection.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Previously, the command was printed by PrepareCommandForRecipe. Letting
ExecCommand print seems more accurate, since it is only printed when it
is actually run (though this already happened in practice).
Additionally, the command can now be modified between
PrepareCommandForRecipe and ExecCommand while preserving correct output.
Since ExecCommand deals with a slice of arguments instead of a single
command string, this requires merging them together into a proper
commandline. Some care is taken to quote arguments containing
spaces, quotes or backslashes, though this is mostly intended for
display purposes. Arguments are only quoted when needed, regardless of
whether they were quoted in the original pattern.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

Usually, the `preproc.macros` recipe includes the C/C++ flags, and
through that the `-MMD` flag to generate dependency files. However,
since include detection passed an output file of `/dev/null` (or the
equivalent on other operating systems), this causes gcc to try and
generate a `/dev/null.d` file and fail.
To prevent this, the `-MMD` flag was filtered out, but this filtering
was applied to the `compiler.cpp.flags` variable, where it *usually*
comes from. However, this is not necessarily true for all platforms. For
example, the PIC32 platform used to have this flag in the
`compiler.c.flags` variable and have `compiler.cpp.flags` include that.
This prevented the flag from being filtered away and caused a failure.
Due to previous changes, it is now possible for this filtering to happen
after all variables have been replaced and the command to run was
generated, but before actually running it. An extra advantage is that
the filtering is more robust than the previous substring-based
filtering.
This fixes arduino#230.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

This slightly simplifies the code.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

This does not use the passed variable yet, but prepares for a future
commit.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

If -debug-level=20 is passed, whenever the cached file is not usable for
whatever reason, a message is displayed. This should help debug caching
problems.
The messages are hardcoded in the source and not put into `constants`,
since they are only debug messages.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

This slightly cleans up a function call.
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>