Page MenuHomePhabricator

[demangler] Simplify OutputBuffer initialization
Needs ReviewPublic

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

Details

Reviewers
dblaikie
tmiasko
Group Reviewers
Restricted Project
Commits
rG5b3ca24a35e9: [demangler] Simplify OutputBuffer initialization
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

Unit TestsFailed

TimeTest
60,030 msx64 debian > libFuzzer.libFuzzer::fuzzer-leak.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LeakTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-leak.test.tmp-LeakTest
60,030 msx64 debian > libFuzzer.libFuzzer::minimize_crash.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/NullDerefTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/minimize_crash.test.tmp-NullDerefTest
60,020 msx64 debian > libFuzzer.libFuzzer::out-of-process-fuzz.test
Script: -- : 'RUN: at line 2'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/out-of-process-fuzz.test.tmp && mkdir /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/out-of-process-fuzz.test.tmp
60,030 msx64 debian > libFuzzer.libFuzzer::value-profile-load.test
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LoadTest.cpp -fsanitize-coverage=trace-gep -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/value-profile-load.test.tmp-LoadTest

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–253

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.Wed, Jun 8, 10:44 AM

Looks good