Graphviz periodically runs into problems where CI test jobs exceed the maximum
timeout and recently the project as a whole has exceeded its Gitlab CI quota.
`test_2095` is by far the longest running component of the test suite. This
change is the result of applying two test case minimizers, one clever¹ and one
not so clever², to the input to this test case. The minimizers were asked to
find smaller input that still (1) provoked a segfault on 588096bd638543ea851ea22751ed91549f61a407 and (2) could be processed successfully
on 32feee561394530713292f8873020fc5feacb9fb. The result takes a ~103KB test
input to ~5KB, with execution time in an example environment dropping from
~155s to <1s.
This dictionary was only ever inserted to by code that was removed in the
previous commit as unreachable. So we know it is always empty and lookups into
it would have always returned null.
graphml2gv: remove dead code handling ending of 'attr' attribute
The code in the last branch in `endElementHandler` was dealing with a closing
`</attr>` tag in the input GraphML. However, the preceding `startElementHandler`
callback rejects an opening `<attr>` tag. So there is no way to reach this
closing logic. This change removes not only the local unreachable code but
several transitively unreachable functions.
As an aside, this was only discovered while trying to construct a test case to
provoke #2300. One path to `dict_relabel` is through `graphml2gv`, by handling a
closing `</attr>` tag. So I looked online for sample GraphML using `attr`. I
could not find any. So I went to the GraphML specification¹ and there discovered
`attr` is not a valid GraphML tag. So I thought, fine, I will figure out how
`graphml2gv` expects this non-standard tag to appear. And that is when I
discovered none of this logic is reachable.
This does not appear to be the result of changes. The very first revision of
`graphml2gv`, 1d28d7d2b4d2b2551bd1f432aa175f54a69364a4, seems to have this
limitation already. I can only conclude this was copy-pasted from gxl2gv without
taking into account the differences between GXL and GraphML.
Boost 1.74.0 deprecates some functions that svgpp is using. Without squashing
these, the CMake build fails in CI:
[ 86%] Building CXX object
tests/CMakeFiles/test_common.dir/svgpp_context.cpp.o
In file included from /usr/include/boost/config/header_deprecated.hpp:18,
from /usr/include/boost/detail/scoped_enum_emulation.hpp:15,
from /usr/include/svgpp/detail/namespace.hpp:13,
from /usr/include/svgpp/detail/attribute_name_to_id.hpp:11,
from
/usr/include/svgpp/attribute_traversal/prioritized.hpp:12,
from
/usr/include/svgpp/attribute_traversal/attribute_traversal.hpp:10,
from /usr/include/svgpp/document_traversal.hpp:10,
from /usr/include/svgpp/svgpp.hpp:1,
from /builds/graphviz/graphviz/tests/svgpp_context.h:7,
from /builds/graphviz/graphviz/tests/svgpp_context.cpp:8:
/usr/include/boost/detail/scoped_enum_emulation.hpp:17:1: note: '#pragma
message: This header is deprecated. Use <boost/core/scoped_enum.hpp>
instead.'
17 | BOOST_HEADER_DEPRECATED("<boost/core/scoped_enum.hpp>")
| ^~~~~~~~~~~~~~~~~~~~~~~
The default toolchain on Ubuntu 22.10 comes with a slightly more pedantic header
layout:
[ 87%] Building CXX object tests/CMakeFiles/test_common.dir/svg_element.cpp.o
tests/svg_element.cpp: In member function 'SVG::SVGRect
SVG::SVGElement::outline_bbox(bool)':
tests/svg_element.cpp:287:29: error: 'all_of' is not a member of 'std'
287 | auto is_vertical = std::all_of(
| ^~~~~~
tests/svg_element.cpp:290:31: error: 'all_of' is not a member of 'std'
290 | auto is_horizontal = std::all_of(
| ^~~~~~
remove 'pointfof' and replace with aggregate initialization
Now that Graphviz is compiled with C99, there does not seem to be much advantage
in retaining this helper function. We have initialization syntax that is the
same number of characters and can be understood locally without having to lookup
the definition of `pointfof`.
unconditionally use 'deflateBound' when zlib is enabled
When Graphviz was built with support for zlib-based compression, it would only
use `deflateBound` if it was discovered at build time. The `deflateBound` code
path provides a tighter estimate of final compressed size, allowing compression
to minimize its intermediate heap allocations.
The CMake build system did not attempt to discover the presence of
`deflateBound` (config-cmake.in). The Windows build hard coded `deflateBound` as
unavailable (windows/include/config.h). So both these builds would result in a
Graphviz that would use more memory than necessary when writing compressed
output formats.
`deflateBound` arrived in zlib 1.2.0, released on 2003-03-09. It seems safe to
unconditionally assume its existence in 2022. The estimate `deflateBound`
provides has improved in successive releases, with even the latest zlib 1.2.13
released on 2022-10-13 further tightening its estimate. So it is expected that
Graphviz users, with this change, will get lower memory usage during writing
compressed formats, and then lower still as newer zlib releases propagate
through the ecosystem.
Calls to this function are generated by Flex – it normally generates calls to
`yyerror` but we alter the Flex prefix to `aag` – but it does not generate a
prototype for the function. So squash the compiler’s overly cautious warning:
../../lib/cgraph/scan.l: At top level:
../../lib/cgraph/scan.l:219:6: warning: no previous prototype for ‘aagerror’
[-Wmissing-prototypes]
219 | void aagerror(const char *str)
| ^~~~~~~~
../../lib/cgraph/scan.l: In function ‘aagerror’:
../../lib/cgraph/scan.l:232:14: warning: switch missing default case
[-Wswitch-default]
232 | else switch (YYSTATE) {
| ^~~~~~
This switch is adding more information to the error message based on which
lexing state we are in, of which four are defined: `INITIAL` (the built-in start
state), `comment`, `hstring`, and `qstring`. In the `INITIAL` state there is no
extra information to add.
cgraph storeFileName: squash warnings when dealing with buffer lengths
This removes the following compiler warnings which were particularly problematic
as they were pointing to the wrong source lines. It is unclear whether this is
due to a Flex bug or the way we are calling Flex.
../../lib/cgraph/scan.l: In function ‘storeFileName’:
../../lib/cgraph/scan.l:98:28: warning: conversion to ‘size_t’ {aka ‘long
unsigned int’} from ‘int’ may change the sign of the result
[-Wsign-conversion]
98 | static char* buf;
| ^
../../lib/cgraph/scan.l:98:37: warning: conversion to ‘size_t’ {aka ‘long
unsigned int’} from ‘int’ may change the sign of the result
[-Wsign-conversion]
98 | static char* buf;
| ^
../../lib/cgraph/scan.l: In function ‘ppDirective’:
../../lib/cgraph/scan.l:125:29: warning: conversion from ‘long int’ to ‘int’
may change value [-Wconversion]
125 | while (*e && *e != '"') e++;
| ~^~
greg [Fri, 7 Oct 2022 00:34:47 +0000 (17:34 -0700)]
Increase stack size for dot MSBUILD project file (avoid crash on larger graphs)
Comment from Matthew Fernandez: It is believed this resolves some instances of
stack overflow in `dfs_enter_outedge` experienced with larger graphs on Windows.
Co-authored-by: Matthew Fernandez <matthew.fernandez@gmail.com>
Return value
…
The currently selected filename, or NULL if no file is selected, or the
selected file can’t be represented with a local filename. Free with g_free().
The caller of the method takes ownership of the data, and is responsible for
freeing it.
Contrary to this, `savefiledlg` was duplicating the pointed to data and then
losing the pointer `gtk_file_chooser_get_filename` returned.
The straightforward fix to this would be to retain the pointer and then `g_free`
it after copying its contents into the buffer `xbuf`. However, we can instead
refactor this function to avoid the copy altogether and simply pass the original
memory back to the caller, making this both a fix and an optimization.
On top of this, `on_gvprbuttonsave_clicked` was never calling `agxbfree` on its
local `xbuf`. By moving to a standard string, we also fix this memory leak.
Return value
…
The currently selected filename, or NULL if no file is selected, or the
selected file can’t be represented with a local filename. Free with g_free().
The caller of the method takes ownership of the data, and is responsible for
freeing it.
Contrary to this, `openfiledlg` was duplicating the pointed to data and then
losing the pointer `gtk_file_chooser_get_filename` returned.
The straightforward fix to this would be to retain the pointer and then `g_free`
it after copying its contents into the buffer `xbuf`. However, we can instead
refactor this function to avoid the copy altogether and simply pass the original
memory back to the caller, making this both a fix and an optimization.
graphml2gv: remove 'composite_buffer' from 'userdata_t'
1d28d7d2b4d2b2551bd1f432aa175f54a69364a4 seems to have copied this
implementation of `userdata_t` from cmd/tools/gxl2gv.c without noticing the
`composite_buffer` member is never used in graphml2gv.
graphml2gv: remove 'xml_attr_value' from 'userdata_t'
1d28d7d2b4d2b2551bd1f432aa175f54a69364a4 seems to have copied this
implementation of `userdata_t` from cmd/tools/gxl2gv.c without noticing the
`xml_attr_value` member is never written to in graphml2gv.
graphml2gv: remove 'xml_attr_name' from 'userdata_t'
1d28d7d2b4d2b2551bd1f432aa175f54a69364a4 seems to have copied this
implementation of `userdata_t` from cmd/tools/gxl2gv.c without noticing the
`xml_attr_name` member is never written to in graphml2gv.
This allows dropping several workarounds in the CMake build system. The only
supported platform with a CMake version less than 3.13.0 is Ubuntu 18.04. Given
CMake is not the default build system, the age of Ubuntu 18.04 makes it unlikely
there are users there building Graphviz from source, and later CMake versions
are easily obtainable from Kitware, it seems reasonable to increase this
requirement.
The `rsvg_term` function has been a no-op for some time:¹
rsvg_term has been deprecated since version 2.36 and should not be used in
newly-written code.
There is no need to de-initialize librsvg.
This function does nothing.
Graphviz was calling it when linked against librsvg 2.36 (2.36.0 was released
2012-03-26). I do not know why this code was conditional as `rsvg_term` has been
a no-op since 2.35.0.² Moreover, calling it prior to it becoming a no-op was
wrong too. The commit that removed its functionality notes:
rsvg_term() was dangerous to call
The problem is that it previously called libxml2’s `xmlCleanupParser` whose
documentation states:³
This function name is somewhat misleading. It does not clean up parser state,
it cleans up memory allocated by the library itself. It is a cleanup function
for the XML library. It tries to reclaim all related global memory allocated
for the library processing. It doesn't deallocate any document related memory.
One should call xmlCleanupParser() only when the process has finished using
the library and all XML/HTML documents built with it. See also xmlInitParser()
which has the opposite function of preparing the library for operations.
WARNING: if your application is multithreaded or has plugin support calling
this may crash the application if another thread or a plugin is still using
libxml2. It's sometimes very hard to guess if libxml2 is in use in the
application, some libraries or plugins may use it without notice. In case of
doubt abstain from calling this function or do it just before calling exit()
to avoid leak reports from valgrind !
This seems to have come to light ~2010.⁴ But presumably there is still a long
tail of software in the wild still, like Graphviz, erroneously calling
`rsvg_term`.
projects / graphviz / log