use fabs() instead of ABS() for taking the absolute value of a double
The fabs() function is available via math.h, even prior to C89 so there's no
reason not to use it. Avoiding the macro eliminates some mental overhead for the
reader. Also fabs has some more lenient semantics with respect to denormal
inputs, so this change results in slightly more efficient code.
Magnus Jacobsson [Sat, 22 Aug 2020 08:27:06 +0000 (10:27 +0200)]
Fix platform name in Windows CI artifacts directory
By renaming $Env:platform to $Env:project_platform. This avoids a
clash with the environment set up with vcvarsall.bat which changes
$Env:platform to the *architecture* which is 'x86' or 'x64' while we
use 'Win32' or 'x64' as the *project-level* platform.
From
https://docs.microsoft.com/en-us/visualstudio/ide/how-to-configure-projects-to-target-platforms?view=vs-2019:
"Note
The Win32 platform name is used for C++ projects, and it means
x86. Visual Studio considers both project-level platforms and
solution-level platforms, and the project platforms come from the
language-specific project systems. C++ projects use Win32 and x64, but
the solution platforms use x86 and x64. When you choose x86 as the
solution configuration, Visual Studio selects the Win32 platform for
C++ projects."
Magnus Jacobsson [Sun, 23 Aug 2020 08:25:50 +0000 (10:25 +0200)]
Make parsing of vcvarsall.bat output more robust
Only consider variable names with alpanumeric characters, underscore
and parentheses. This avoids most problems when parsing output
containing multi-line variable values with arbitrary text.
In GitLab CI, one of the environment variables is CI_COMMIT_MESSAGE. In the commit immediately following this commit there is an URL containing an equal sign that caused the error below:
Set-Content : An object at the specified path
env:\https://docs.microsoft.com/en-us/visualstudio/ide/how-to-configure-projects-to-target-platforms?view does not
exist, or has been filtered by the -Include or -Exclude parameter.
At line:3 char:5
+ Set-Content "env:\$($matches[1])" $matches[2];
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : ObjectNotFound: (System.String[]:String[]) [Set-Content], Exception
+ FullyQualifiedErrorId : ItemNotFound,Microsoft.PowerShell.Commands.SetContentCommand
[ 69%] Building C object lib/cgraph/CMakeFiles/cgraph.dir/grammar.c.o
[ 70%] Building C object lib/cgraph/CMakeFiles/cgraph.dir/scan.c.o
/Users/north/src/graphviz/build/lib/cgraph/scan.c:1706:12: error: expected
identifier or '('
extern int isatty (int );
^
scan.l:44:19: note: expanded from macro 'isatty'
#define isatty(x) 0
^
1 error generated.
make[2]: *** [lib/cgraph/CMakeFiles/cgraph.dir/scan.c.o] Error 1
make[1]: *** [lib/cgraph/CMakeFiles/cgraph.dir/all] Error 2
make: *** [all] Error 2
which conveniently explained why this find-and-replace had existed in the first
place. Rather than reverting this, the present change uses a more principled way
of instructing Flex not to call isatty().
Commits 02d2cacf8730f1e4347afd72b159c91fafed2182 and 80a9dc3a3d7a1bff49db3f8e40fc03e91ca2261e accidentally introduced two new
compiler warnings to the build. Following this change, the CMake build returns
to warning-free on Linux with GCC 8.3.0. The Autotools build still sprays
warnings, but I think it's worth taking proactive steps like this to never
introduce new warnings.
Though this change looks like it has a semantic effect, it does not. The base
member of these structs is the first member, used as an inheritance-style
pattern.
use a dynamic buffer when expanding text replacements
This change is intended to guard against bugs like that fixed in fbefeb31989130c48b965cc9a2f0ad0cac07015c. The buffer used to construct the
result string now dynamically expands so we do not need to manually calculate
its extent in advance. Related to #1676.
Instead of retrieving the contents of a (possibly dynamically allocated) buffer
and then allocating new space for a copy, we can simply take ownership of the
original buffer. This saves an extra unnecessary allocation.
Magnus Jacobsson [Fri, 14 Aug 2020 12:55:30 +0000 (14:55 +0200)]
Make the Windows pipeline stop on PowerShell cmdlet failure
Note that this doesn't affect failures from .exe programs (but that is
already handled by the GitLab Shared Windows Runner). See
https://stackoverflow.com/questions/9948517/how-to-stop-a-powershell-script-on-the-first-error.
fix incorrect allocation computation in strdup_and_subst_obj0
The logic in this function does two passes through computing the construction of
a string, the first to determine how many bytes will be written and the second
to write those bytes. Unfortunately the logic in these two passes did not match,
and some cases would cause the first pass to underestimate how many bytes would
be written by the second pass. As a result, the second pass would overrun the
extent of allocated memory and begin writing out of bounds.
As far as I can tell, this bug has existed since at least 2010. I think this
code should be refactored in future to use a dynamically expanding string buffer
(e.g. agxbuf) instead of relying on this fragile pairing of logic.
The test case added in this commit was something found by Google Autofuzz
project that triggered a crash in Graphviz due to reading a NULL pointer. After
fixing the initial crash (the previous commit), this out of bounds write was
observable using ASan. This commit fixes #1676.
fix: avoid accessing min/max rep on a NULL cluster
When entering compile_samerank, the parent cluster could be NULL, which was then
incorrectly dereferenced in the two cases affected in this change. This issue
was found by Google Autofuzz project. Related to #1676.
Magnus Jacobsson [Fri, 14 Aug 2020 10:04:08 +0000 (12:04 +0200)]
Fix publishing of Windows build artifcats
The COLLECTION was missing since we no longer get it from the
portable_source stage because the Windows builds start directly and do
not wait for the portable_source stage. Therefor we generate it in the
Windows builds directly instead.
Magnus Jacobsson [Fri, 14 Aug 2020 09:53:22 +0000 (11:53 +0200)]
Change to all-numeric version entry format and add collection
The version is now specified with a new collection variable which
shall be "stable" or "development" and an all-numeric version. The
"~dev" (and the .<commiter date>) suffix will be added by the script.
The distance between endb and next was being stored in an int variable. While
this should not exceed the bounds of an int during normal usage, it's risky to
store a pointer difference in a narrower type this way.
fix: pass format parameter into gverrorf explicitly
The gverrorf function was prototyped in a way that did not align with a function
pointer it was stored into. On platforms with more forgiving varargs calling
conventions, everything worked out OK. However, on other platforms calling
gverrorf through this function pointer would result in stack corruption. This
change aligns gverrorf with the type of the function pointer in which it is
stored.
change errorv calling convention to explicitly take a format string
This function implicitly assumed its variable arguments contained a format
string as the first argument. This change pushes the assumption into the calling
convention, making it visible to the compiler.
This spin lock incorrectly implied to the reader that this code was thread safe.
IMHO there are two primary issues with this spin lock and one secondary issue:
1. The intention of the spin lock is clearly to provide mutual exclusion
within fmtbuf(). However this function returns a pointer to a static
(thread-global) buffer. Excluding two threads from being in this function
concurrently does not help if both exit the function with pointers into the
same memory.
2. The spin lock uses no atomics or volatile. As a result, accesses to the
lock can be freely rearranged or coalesced, with no regard to thread
interleaving. This can result in one thread never seeing another thread's
update (unlock) to the lock. Even worse, my local compiler simply
recognizes what is happening and omits the lock operations entirely.
3. (secondary) If this lock worked as intended, it would spin writing to the
lock variable. This would cause fairly pessimistic cache line bouncing as
all waiting processors try to keep the line containing the lock in
exclusive state in their local cache. A more performant way to achieve
this is to spin waiting to see the lock value change *without* modifying
the lock yourself. This allows multiple waiting processors to have the line
in shared state, and minimizes cache coherence traffic.
Certain edge attributes are constructed in advance of their being seen in the
input because Graphviz knows it may need default values for them. Later, if seen
in the input, the values of these attributes are updated.
This all works fine unless the order in which these initially-defaulted edge
attributes appear in the input does not match the order in which the default
versions are constructed by Graphviz internally. In this case, the order in
which the attributes are seen in the input is used to construct a dictionary of
them, but the original copies are used to index into attribute values.
In the particular test case added in this commit,
digraph {
{ rank=same; n1; n2 }
n2 -> n1 [ headport=s, arrowhead=normal ]
}
arrowhead was constructed with symbol ID 0 and headport was constructed with
symbol ID 1. But then the later parsing of these attributes resulted in a
dictionary where the headport value was in ID 0 and the arrowhead value was in
ID 1. Indexing into this dictionary with the initially constructed E_arrowhead
resulted in incorrectly returning the value "s". This caused a spurious error
'Arrow type "s" unknown' as well as incorrect graph output.
Fixes #1444. Note that this may just be one of several issues resulting from
using these initially constructed E_* symbols.
projects / graphviz / log