Pango plugin: fix: do not judge empty lines as failing during text layout
Text layout plugins are expected to return failure as `false` from their
`textlayout` function. The Pango plugin considered failure to be anything that
resulted in a horizontal layout width of 0. However, this is not a failure in
the case where the text being laid out is the empty string; its horizontal width
is expected to be 0.
The effect of this was that HTML-like strings like `<<br/>1>` were judged to
fail during text layout and a (redundant) estimation of their text width was
performed. This seems to have been a latent bug present since commit ad82ef8613b0731806b81f9c0047d0cbf6745470 (~July 2007). This recently became more
visible due to commit 7aa0dcc03ea20b544b2463d97fe4a78af699589c that introduced
warnings during text width estimation if a fallback metric needed to be used.
Users were now presented with “Warning: no hard-coded metrics” when using fonts
that Pango knew of and should not have needed estimation in the first place.
This fix makes the Pango plugin consider 0 width for the layout of an empty
string to be successful. To be clear, this commit is both a functional fix and a
performance improvement.
CMake: enable libgd support on macOS when possible
As discussed in the comment in this commit, we hard code the options the
Homebrew libgd package is built with.¹ It seems not possible to build a libgd
without GIF support, so perhaps the `HAVE_GD_GIF` logic in the Graphviz code
base could be simplified in future.
CMake: fix: link against pathplan when building GD plugin
This replicates something from the Autotools build system that was missing in
the CMake build system. The CMake build was not failing because the pathplan
dependency is only required when VRML support is enabled, which an upcoming
commit will add.
In contrast to the Autotools build system, this adds pathplan for all platforms,
not only Windows. Empirically it seems necessary on at least macOS and Windows
and it does no harm on Linux.
This is mostly just over-cautiousness to not accidentally define variables like
`HAVE_GD_PNG` if we not have libgd. But it will also ease some upcoming
macOS-specific changes.
CI: skip package uploading and deployment steps on non-release revisions
Traditionally CI has uploaded release artifacts and packaged a “release” for
every single commit on the main branch. These inter-release packages were
intended for testing and internal deployment. As far as we aware these are no
longer used; all Graphviz developers build from source and do not rely on
Gitlab’s generic package repository.¹
This change rearranges deployment steps to skip uploading artifacts and
packaging if the current commit is not a proper release. This should slightly
accelerate CI and reduce Graphviz storage requirements, which are currently at
>800GB on Gitlab.
fix gvpr corruption of dynamically allocated arguments to user-defined functions
Gvpr programs can define their own functions which can then be called within the
same program:
void foo(string s) {
print(s);
}
foo("bar");
This mostly worked. However in some cases the gvpr implementation was not
extending the lifetime of the memory allocated to store the passed in value
long enough. Enumerating the cases in which this occurred is complicated because
whether this (used-after-free) memory retained its intended content depended on
(1) the complexity of the expression of the passed in value and (2) what the
target function (`foo` in the above example) was itself doing. As a result, it
seems users mostly did not observe the problem (program/output corruption)
unless they were writing non-trivial functions and calling them with non-trivial
expressions.
Commit 8da53964edec8a665c3996d483df243eb150c2c4 compounded the above problem by
replacing the underlying allocator. While both before and after states use an
arena allocator,¹ the allocator after this change eagerly returns memory to the
backing system allocator (`malloc`) on `vmclear` while the allocator before this
change retained it within its own internal pool. The system allocator is used
much more pervasively in the Graphviz code base than the more tightly scoped
lib/vmalloc allocator, and it also typically does much more aggressive reuse of
recently-freed memory under the assumption that this is more likely to still be
cache-resident and thus faster to access. The net effect of this was that the
chance of the memory in question being reused and overwritten significantly
increased, making a number of latent cases of the problem described above now
user-visible.
The fix in this commit removes the freeing of expressions that are still
potentially in use. The contents of a subexpression in the above described
scenarios now remains intact up to the point it is accessed when evaluating its
parent containing expression.
The astute reader who has followed everything up to now may notice that the
subexpressions’ contents are actually maintained _beyond_ the point of
evaluation of the parent expression, and may be wondering, “didn’t you just turn
a use-after-free into a memory leak?” Unfortunately the answer is yes. However,
it is unclear how to determine when it is safe to free a subexpression without
introducing a more complex concept of call stacks and arbitrarily nested
expressions to lib/expr. Thus given the choice right now between use-after-free
or leaking memory, we are choosing to leak memory. Hopefully this can be
revisited in future.
cgraph: fix: handle allocation failures in 'agcanon' and friends
There are a number of cgraph API functions that use an internally managed buffer
to save the caller from having to allocate space themselves. Failure to expand
this buffer was being silently ignored, resulting in messy crashes when memory
was exhausted. These failures are now checked for and `agcanon` and `agcanonStr`
return `NULL` on allocation failure. `agwrite` returns `EOF` on allocation
failure.
This code was accepting large negative numbers and then converting them to
positive numbers that were applied as the line length limit. This seems clearly
unintended. This rephrasing now ignores any out of range value set for
`linelength`.
Windows: remove usage of 'sed' in setup-build-utilities script
This results in `$PATH` potentially having duplicated trailing entries, but this
causes no harm and preventing it does not seem worth a `sed` dependency.
Costa Shulyupin [Tue, 22 Feb 2022 18:58:05 +0000 (20:58 +0200)]
lib/cgraph: use return value of 'fread' in aglasterr
fix warning:
agerror.c: In function ‘aglasterr’:
agerror.c:49:5: warning: ignoring return value of ‘fread’, declared with attribute warn_unused_result [-Wunused-result]
49 | fread(buf, sizeof(char), len, agerrout);
This code was attempting to avoid dynamic string construction with low numbered
IDs. This is unnecessary when using a modern compiler that understands functions
like `snprintf` as built-ins and can achieve better optimization without this
trick.
Apart from de-duplicating code, this has the effect of making this code thread
safe where it was not before.
Magnus Jacobsson [Sun, 20 Feb 2022 10:10:04 +0000 (11:10 +0100)]
gvc++: include "AGraph.h" instead of <AGraph.h> in GVLayout.h
This makes it possible to use GVLayout.h from the install directory
without the need to specify both "include" and "include/graphviz" as
include directories to the compiler.
Magnus Jacobsson [Sun, 20 Feb 2022 10:08:52 +0000 (11:08 +0100)]
gvc++: include "gvc.h" instead of <gvc.h> in GVContext.h
This makes it possible to use GVContext.h from the install directory
without the need to specify both "include" and "include/graphviz" as
include directories to the compiler.
Towards https://gitlab.com/graphviz/graphviz/-/issues/2196.
Magnus Jacobsson [Sun, 20 Feb 2022 10:03:43 +0000 (11:03 +0100)]
cgraph++: include "cgraph.h" instead of <cgraph.h> in AGraph.h
This makes it possible to use AGraph.h from the install directory
without the need to specify both "include" and "include/graphviz" as
include directories to the compiler.
Towards https://gitlab.com/graphviz/graphviz/-/issues/2196.
projects / graphviz / log