This is an archive of the discontinued LLVM Phabricator instance.

[demangler] Simplify OutputBuffer initialization
ClosedPublic

Authored by urnathan on Mar 28 2022, 11:32 AM.

Details

Summary

This next cleanup of the demangler's OutputBuffer class addresses the initial allocation of the output buffer. Now that the buffer extension logic has hysteresis in it, there's no need for every use of OutputBuffer to explicitly create the initial buffer (using either 128 or 1024 as initial guesses).

For the default case we can rely on the buffer extension logic to create a good initial buffer. For the cases where the user is reusing a buffer, pass that information into the constructor. Thus the OutputBuffer's ownership of the buffer starts at its own lifetime start -- a nice bit of RAII. We can reduce the lifetime of this object in several cases.

That new constructor takes a 'size_t *' for the size argument, as all uses with a non-null buffer are passing through a malloc'd buffer from their own caller in this manner.

The buffer reset member function is never used, and is deleted.

The original buffer initialization code would return a failure code if that first malloc failed. Existing code either ignored that, called std::terminate with a FIXME, or returned an error code.

But that's not foolproof anyway, as a subsequent buffer extension failure ends up calling std::terminate. I am working on addressing that unfortunate failure mode in a manner more consistent with the C++ ABI design.

I have a patch dealing with the destructing part of this API coming up.

Diff Detail

Event Timeline

urnathan created this revision.Mar 28 2022, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 11:32 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
urnathan requested review of this revision.Mar 28 2022, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 11:32 AM
urnathan edited the summary of this revision. (Show Details)Mar 31 2022, 9:46 AM
urnathan edited reviewers, added: Restricted Project; removed: erichkeane.
urnathan updated this revision to Diff 420558.Apr 5 2022, 9:42 AM

re-rebase

urnathan edited reviewers, added: tmiasko; removed: bruno.Apr 11 2022, 11:35 AM

Tomasz, your comments on the rust demangler were most helpful. Perhaps you have bandwidth to look at this?

urnathan updated this revision to Diff 422574.Apr 13 2022, 11:15 AM
urnathan edited the summary of this revision. (Show Details)

Rebased ontop of D123420

dblaikie accepted this revision.Apr 15 2022, 10:28 AM

Sounds OK to me

This revision is now accepted and ready to land.Apr 15 2022, 10:28 AM
This revision was landed with ongoing or failed builds.Apr 26 2022, 4:23 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 4:23 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript

Looks like it night have broken a sanitizer build: https://lab.llvm.org/buildbot/#/builders/5/builds/22738

Yes, I got a note about that. Fix will be available shortly.

urnathan reopened this revision.May 2 2022, 4:49 AM
urnathan updated this revision to Diff 426394.May 2 2022, 6:22 AM

rebased an updated to avoid the UB discovered in the earlier version.

I went with @dblaikie's suggestion of detecting the UB at point of use. This makes D124524 moot.

dblaikie added inline comments.May 2 2022, 11:13 AM
llvm/lib/Demangle/MicrosoftDemangle.cpp
249–250

Looks good to me as an alternative to D124524

llvm/unittests/Demangle/OutputBufferTest.cpp
19–21 ↗(On Diff #426394)

I don't think this check is necessary though, is it? nullptr,nullptr should probably be valid iterators, yeah?

urnathan updated this revision to Diff 426648.May 3 2022, 4:44 AM
urnathan updated this revision to Diff 426649.May 3 2022, 4:45 AM
urnathan marked an inline comment as done.

Removed the OutputBufferTest change. I had thought iterators could never be null, but I see I was mistaken.

urnathan updated this revision to Diff 426996.May 4 2022, 7:09 AM

rebase, hopefully fix the CI tester

urnathan updated this revision to Diff 429222.May 13 2022, 6:17 AM

hm, let's try that rebase again

tmiasko accepted this revision.May 13 2022, 8:49 AM

Looks good.

Nit: reviewing from the perspective of OutputBuffer starting empty, the OutputBuffer::prepend might need an extra check to avoid passing nullptr to memmove / memcpy, in an unusual case an empty string were to be prepended to an empty output buffer.

As far as CI is concerned, if you are logged in the Phabricator, then following completed remote builds in B164300: Diff 429222 -> pre-merge checks -> plain text URL https://buildkite.com/llvm-project/diff-checks/builds/104149 shows:

Attention! D124524 is one of the dependencies of the target revision D122604.
No testing is possible because we couldn't apply the patch.

Looks like there is now a stale parent revision D124524 and patch fails to apply.

urnathan updated this revision to Diff 429690.May 16 2022, 5:45 AM

ah, thanks for the clue!

dblaikie accepted this revision.Jun 8 2022, 10:44 AM

Looks good

how can I get libc++abi review here?

@ldionne / @compnerd perhaps one of you could take a quick look?

ping pingedy ping?

I think it's OK to go ahead with this without further approval - the phab marker here is more of a guideline than a rule, I believe. This isn't breaking/touching any part of the public API, etc, so is probably less in need of scrutiny.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 17 2022, 4:35 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.