Update of bug #68585 (group groff):
Assigned to: None => gbranden
Planned Release: None => 1.25.0
Summary: [pic,tbl] code not
"ASAN_OPTIONS=alloc_dealloc_mismatch=1" clean => [eqn,pic,tbl] code not
"ASAN_OPTIONS=alloc_dealloc_mismatch=1" clean
_______________________________________________________
Follow-up Comment #4:
Hi Bruno,
[comment #3 comment #3:]
> Find attached five patches, that fix all the ASAN issues in a "make -k check"
> run.
Outstanding!
> Some ASAN findings became visible only after other ASAN findings were fixed,
> that's why it needs five patches and not only two.
Yup, I'm not surprised at all. Fear of revision scope creep and risk of
imposing delay on the release schedule--we've already missed the "early July"
chronological objective, which is much more my fault than anyone else's--is
why I shied away from making resolution of this/these issue(s) a release goal
for 1.25.0.
Not that _groff_, officially, _has_ "release goals".
https://lists.gnu.org/archive/html/groff/2026-06/msg00017.html
But your patches show that the impact radius of resolving this mildly
worrisome issue is tightly constrained, utterly unlike my ramifying yarn-pull
of comment #1.
Thank you!
> All that was needed is to call free(x) instead of delete[] x in a few places.
> And updating comments, to avoid future mistakes of the same kind. Note that
> for a similar case, there was already a comment
> // `malloc()`ed by `string::extract()`
> in src/preproc/tbl/table.cpp.
Yes. You found a bread crumb I had intended to follow up on in the indefinite
future. :)
There are usually several potential modernizations or technical debt paydowns
I can see before me in any given screenful of _groff_ code. It's challenging
to stay on one such issue long enough to eradicate it. What more often
happens is that I whack away at a breadth-based refactoring (vs. a depth-based
one, where I heavily revise a single function) a few greps at a time over
months or years, interleaved with the other issues I work on. Eventually, it
gets done. Savannah tickets in Item Group "Refactoring" and tagged with
"[long-term]" in the Summary are examples of these.
>> trying to resolve the tbl issue pulled on a long thread of
>> `const`-incorrectness and an insufficiently well documented internal API for
>> libgroff's "string" class.
>
>> One will observe that diff changed the return type of `string::extract()`
>> away from the "reform" contemplated above. That's because I worked
>> backwards from how tbl was using it.
>
> The changes that you are considering here are not needed for resolving the
> ASAN failures. No 'const' / non-'const' change and no function signature
> change is needed for this ticket.
I concur. Moving some internals "back" to libc memory management functions is
not a direction I wanted to go since I desired to move the code base purely to
the C++ language allocator instead.
But allocator UI consistency, and ASAN cleanliness, is a much more significant
virtue than occasional, but symmetric, libc allocator usage is a vice.
Moreover, your changes don't make the eventual "forward migration" of the
sites of allocator usage significantly more difficult. Any such revision
should involve a coupled check of both the allocation and deallocation sites
anyway, to ensure that the developer comprehends the change and sniffs for
code smells.
I'll apply these changes to our master branch and expect them to ship in
_groff_ 1.25.0 final.
Thank you again!
_______________________________________________________
Reply to this item at:
<https://savannah.gnu.org/bugs/?68585>
_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/
signature.asc
Description: PGP signature
