[clang-tools-extra] r333993 - [clangd] Rewrite JSON dispatcher loop using C IO (FILE*) instead of std::istream.
Kostya Serebryany via cfe-commits
cfe-commits at lists.llvm.org
Fri Jun 8 13:37:09 PDT 2018
thanks!
On Fri, Jun 8, 2018 at 1:31 PM Sam McCall <sam.mccall at gmail.com> wrote:
> Oops, thank you!
> r334315 should fix this.
>
>
>
> On Fri, Jun 8, 2018 at 9:45 PM Kostya Serebryany <kcc at google.com> wrote:
>
>> Looks like this broke the clang-fuzzer:
>> https://oss-fuzz-build-logs.storage.googleapis.com/index.html
>>
>> Step #4: /src/llvm/tools/clang/tools/extra/clangd/fuzzer/ClangdFuzzer.cpp:31:17: error: no viable conversion from 'std::istringstream' (aka 'basic_istringstream<char>') to 'std::FILE *' (aka '_IO_FILE *')
>> Step #4: LSPServer.run(In);
>> Step #4: ^~
>> Step #4: /src/llvm/tools/clang/tools/extra/clangd/fuzzer/../ClangdLSPServer.h:46:23: note: passing argument to parameter 'In' here
>> Step #4: bool run(std::FILE *In,
>> Step #4: ^
>> Step #4: 1 error generated.
>> Step #4: ninja: build stopped: subcommand failed.
>>
>>
>>
>> On Tue, Jun 5, 2018 at 2:38 AM Sam McCall via cfe-commits <
>> cfe-commits at lists.llvm.org> wrote:
>>
>>> Author: sammccall
>>> Date: Tue Jun 5 02:34:46 2018
>>> New Revision: 333993
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=333993&view=rev
>>> Log:
>>> [clangd] Rewrite JSON dispatcher loop using C IO (FILE*) instead of
>>> std::istream.
>>>
>>> Summary:
>>> The EINTR loop around getline was added to fix an issue with mac gdb,
>>> but seems
>>> to loop infinitely in rare cases on linux where the parent editor exits
>>> (most
>>> reports with VSCode).
>>> I can't work out how to fix this in a portable way with std::istream,
>>> but the
>>> C APIs have clearer contracts and LLVM has a RetryAfterSignal function
>>> for use
>>> with them which seems battle-tested.
>>>
>>> While here, clean up some inconsistency around \n in log messages (now
>>> add it only after JSON payloads), and reduce the scope of the
>>> long-message handling which was only really added to fight fuzzers.
>>>
>>> Reviewers: malaperle, ilya-biryukov
>>>
>>> Subscribers: klimek, ioeric, jkorous, cfe-commits
>>>
>>> Differential Revision: https://reviews.llvm.org/D47643
>>>
>>> Modified:
>>> clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
>>> clang-tools-extra/trunk/clangd/ClangdLSPServer.h
>>> clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
>>> clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
>>> clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
>>> clang-tools-extra/trunk/test/clangd/too_large.test
>>>
>>> Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=333993&r1=333992&r2=333993&view=diff
>>>
>>> ==============================================================================
>>> --- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
>>> +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Tue Jun 5
>>> 02:34:46 2018
>>> @@ -396,7 +396,7 @@ ClangdLSPServer::ClangdLSPServer(JSONOut
>>> SupportedSymbolKinds(defaultSymbolKinds()),
>>> Server(CDB, FSProvider, /*DiagConsumer=*/*this, Opts) {}
>>>
>>> -bool ClangdLSPServer::run(std::istream &In, JSONStreamStyle InputStyle)
>>> {
>>> +bool ClangdLSPServer::run(std::FILE *In, JSONStreamStyle InputStyle) {
>>> assert(!IsDone && "Run was called before");
>>>
>>> // Set up JSONRPCDispatcher.
>>>
>>> Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.h?rev=333993&r1=333992&r2=333993&view=diff
>>>
>>> ==============================================================================
>>> --- clang-tools-extra/trunk/clangd/ClangdLSPServer.h (original)
>>> +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h Tue Jun 5 02:34:46
>>> 2018
>>> @@ -42,8 +42,8 @@ public:
>>> /// class constructor. This method must not be executed more than
>>> once for
>>> /// each instance of ClangdLSPServer.
>>> ///
>>> - /// \return Wether we received a 'shutdown' request before an 'exit'
>>> request
>>> - bool run(std::istream &In,
>>> + /// \return Whether we received a 'shutdown' request before an 'exit'
>>> request.
>>> + bool run(std::FILE *In,
>>> JSONStreamStyle InputStyle = JSONStreamStyle::Standard);
>>>
>>> private:
>>>
>>> Modified: clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp?rev=333993&r1=333992&r2=333993&view=diff
>>>
>>> ==============================================================================
>>> --- clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp (original)
>>> +++ clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp Tue Jun 5
>>> 02:34:46 2018
>>> @@ -14,6 +14,7 @@
>>> #include "llvm/ADT/SmallString.h"
>>> #include "llvm/ADT/StringExtras.h"
>>> #include "llvm/Support/Chrono.h"
>>> +#include "llvm/Support/Errno.h"
>>> #include "llvm/Support/SourceMgr.h"
>>> #include <istream>
>>>
>>> @@ -66,7 +67,7 @@ void JSONOutput::writeMessage(const json
>>> Outs << "Content-Length: " << S.size() << "\r\n\r\n" << S;
>>> Outs.flush();
>>> }
>>> - log(llvm::Twine("--> ") + S);
>>> + log(llvm::Twine("--> ") + S + "\n");
>>> }
>>>
>>> void JSONOutput::log(const Twine &Message) {
>>> @@ -180,27 +181,43 @@ bool JSONRPCDispatcher::call(const json:
>>> return true;
>>> }
>>>
>>> -static llvm::Optional<std::string> readStandardMessage(std::istream &In,
>>> +// Tries to read a line up to and including \n.
>>> +// If failing, feof() or ferror() will be set.
>>> +static bool readLine(std::FILE *In, std::string &Out) {
>>> + static constexpr int BufSize = 1024;
>>> + size_t Size = 0;
>>> + Out.clear();
>>> + for (;;) {
>>> + Out.resize(Size + BufSize);
>>> + // Handle EINTR which is sent when a debugger attaches on some
>>> platforms.
>>> + if (!llvm::sys::RetryAfterSignal(nullptr, ::fgets, &Out[Size],
>>> BufSize, In))
>>> + return false;
>>> + clearerr(In);
>>> + // If the line contained null bytes, anything after it (including
>>> \n) will
>>> + // be ignored. Fortunately this is not a legal header or JSON.
>>> + size_t Read = std::strlen(&Out[Size]);
>>> + if (Read > 0 && Out[Size + Read - 1] == '\n') {
>>> + Out.resize(Size + Read);
>>> + return true;
>>> + }
>>> + Size += Read;
>>> + }
>>> +}
>>> +
>>> +// Returns None when:
>>> +// - ferror() or feof() are set.
>>> +// - Content-Length is missing or empty (protocol error)
>>> +static llvm::Optional<std::string> readStandardMessage(std::FILE *In,
>>> JSONOutput &Out)
>>> {
>>> // A Language Server Protocol message starts with a set of HTTP
>>> headers,
>>> // delimited by \r\n, and terminated by an empty line (\r\n).
>>> unsigned long long ContentLength = 0;
>>> - while (In.good()) {
>>> - std::string Line;
>>> - std::getline(In, Line);
>>> - if (!In.good() && errno == EINTR) {
>>> - In.clear();
>>> - continue;
>>> - }
>>> + std::string Line;
>>> + while (true) {
>>> + if (feof(In) || ferror(In) || !readLine(In, Line))
>>> + return llvm::None;
>>>
>>> Out.mirrorInput(Line);
>>> - // Mirror '\n' that gets consumed by std::getline, but is not
>>> included in
>>> - // the resulting Line.
>>> - // Note that '\r' is part of Line, so we don't need to mirror it
>>> - // separately.
>>> - if (!In.eof())
>>> - Out.mirrorInput("\n");
>>> -
>>> llvm::StringRef LineRef(Line);
>>>
>>> // We allow comments in headers. Technically this isn't part
>>> @@ -208,19 +225,13 @@ static llvm::Optional<std::string> readS
>>> if (LineRef.startswith("#"))
>>> continue;
>>>
>>> - // Content-Type is a specified header, but does nothing.
>>> - // Content-Length is a mandatory header. It specifies the length of
>>> the
>>> - // following JSON.
>>> - // It is unspecified what sequence headers must be supplied in, so
>>> we
>>> - // allow any sequence.
>>> - // The end of headers is signified by an empty line.
>>> + // Content-Length is a mandatory header, and the only one we handle.
>>> if (LineRef.consume_front("Content-Length: ")) {
>>> if (ContentLength != 0) {
>>> log("Warning: Duplicate Content-Length header received. "
>>> "The previous value for this message (" +
>>> - llvm::Twine(ContentLength) + ") was ignored.\n");
>>> + llvm::Twine(ContentLength) + ") was ignored.");
>>> }
>>> -
>>> llvm::getAsUnsignedInteger(LineRef.trim(), 0, ContentLength);
>>> continue;
>>> } else if (!LineRef.trim().empty()) {
>>> @@ -233,46 +244,45 @@ static llvm::Optional<std::string> readS
>>> }
>>> }
>>>
>>> - // Guard against large messages. This is usually a bug in the client
>>> code
>>> - // and we don't want to crash downstream because of it.
>>> + // The fuzzer likes crashing us by sending "Content-Length:
>>> 9999999999999999"
>>> if (ContentLength > 1 << 30) { // 1024M
>>> - In.ignore(ContentLength);
>>> - log("Skipped overly large message of " + Twine(ContentLength) +
>>> - " bytes.\n");
>>> + log("Refusing to read message with long Content-Length: " +
>>> + Twine(ContentLength) + ". Expect protocol errors.");
>>> + return llvm::None;
>>> + }
>>> + if (ContentLength == 0) {
>>> + log("Warning: Missing Content-Length header, or zero-length
>>> message.");
>>> return llvm::None;
>>> }
>>>
>>> - if (ContentLength > 0) {
>>> - std::string JSON(ContentLength, '\0');
>>> - In.read(&JSON[0], ContentLength);
>>> - Out.mirrorInput(StringRef(JSON.data(), In.gcount()));
>>> -
>>> - // If the stream is aborted before we read ContentLength bytes, In
>>> - // will have eofbit and failbit set.
>>> - if (!In) {
>>> - log("Input was aborted. Read only " + llvm::Twine(In.gcount()) +
>>> - " bytes of expected " + llvm::Twine(ContentLength) + ".\n");
>>> + std::string JSON(ContentLength, '\0');
>>> + for (size_t Pos = 0, Read; Pos < ContentLength; Pos += Read) {
>>> + // Handle EINTR which is sent when a debugger attaches on some
>>> platforms.
>>> + Read = llvm::sys::RetryAfterSignal(0u, ::fread, &JSON[Pos], 1,
>>> + ContentLength - Pos, In);
>>> + Out.mirrorInput(StringRef(&JSON[Pos], Read));
>>> + if (Read == 0) {
>>> + log("Input was aborted. Read only " + llvm::Twine(Pos) +
>>> + " bytes of expected " + llvm::Twine(ContentLength) + ".");
>>> return llvm::None;
>>> }
>>> - return std::move(JSON);
>>> - } else {
>>> - log("Warning: Missing Content-Length header, or message has zero "
>>> - "length.\n");
>>> - return llvm::None;
>>> + clearerr(In); // If we're done, the error was transient. If we're
>>> not done,
>>> + // either it was transient or we'll see it again on
>>> retry.
>>> + Pos += Read;
>>> }
>>> + return std::move(JSON);
>>> }
>>>
>>> // For lit tests we support a simplified syntax:
>>> // - messages are delimited by '---' on a line by itself
>>> // - lines starting with # are ignored.
>>> // This is a testing path, so favor simplicity over performance here.
>>> -static llvm::Optional<std::string> readDelimitedMessage(std::istream
>>> &In,
>>> +// When returning None, feof() or ferror() will be set.
>>> +static llvm::Optional<std::string> readDelimitedMessage(std::FILE *In,
>>> JSONOutput
>>> &Out) {
>>> std::string JSON;
>>> std::string Line;
>>> - while (std::getline(In, Line)) {
>>> - Line.push_back('\n'); // getline() consumed the newline.
>>> -
>>> + while (readLine(In, Line)) {
>>> auto LineRef = llvm::StringRef(Line).trim();
>>> if (LineRef.startswith("#")) // comment
>>> continue;
>>> @@ -284,39 +294,47 @@ static llvm::Optional<std::string> readD
>>> JSON += Line;
>>> }
>>>
>>> - if (In.bad()) {
>>> + if (ferror(In)) {
>>> log("Input error while reading message!");
>>> return llvm::None;
>>> - } else {
>>> + } else { // Including EOF
>>> Out.mirrorInput(
>>> llvm::formatv("Content-Length: {0}\r\n\r\n{1}", JSON.size(),
>>> JSON));
>>> return std::move(JSON);
>>> }
>>> }
>>>
>>> -void clangd::runLanguageServerLoop(std::istream &In, JSONOutput &Out,
>>> +// The use of C-style std::FILE* IO deserves some explanation.
>>> +// Previously, std::istream was used. When a debugger attached on
>>> MacOS, the
>>> +// process received EINTR, the stream went bad, and clangd exited.
>>> +// A retry-on-EINTR loop around reads solved this problem, but caused
>>> clangd to
>>> +// sometimes hang rather than exit on other OSes. The interaction
>>> between
>>> +// istreams and signals isn't well-specified, so it's hard to get this
>>> right.
>>> +// The C APIs seem to be clearer in this respect.
>>> +void clangd::runLanguageServerLoop(std::FILE *In, JSONOutput &Out,
>>> JSONStreamStyle InputStyle,
>>> JSONRPCDispatcher &Dispatcher,
>>> bool &IsDone) {
>>> auto &ReadMessage =
>>> (InputStyle == Delimited) ? readDelimitedMessage :
>>> readStandardMessage;
>>> - while (In.good()) {
>>> + while (!IsDone && !feof(In)) {
>>> + if (ferror(In)) {
>>> + log("IO error: " + llvm::sys::StrError());
>>> + return;
>>> + }
>>> if (auto JSON = ReadMessage(In, Out)) {
>>> if (auto Doc = json::parse(*JSON)) {
>>> // Log the formatted message.
>>> log(llvm::formatv(Out.Pretty ? "<-- {0:2}\n" : "<-- {0}\n",
>>> *Doc));
>>> // Finally, execute the action for this JSON message.
>>> if (!Dispatcher.call(*Doc, Out))
>>> - log("JSON dispatch failed!\n");
>>> + log("JSON dispatch failed!");
>>> } else {
>>> // Parse error. Log the raw message.
>>> log(llvm::formatv("<-- {0}\n" , *JSON));
>>> log(llvm::Twine("JSON parse error: ") +
>>> - llvm::toString(Doc.takeError()) + "\n");
>>> + llvm::toString(Doc.takeError()));
>>> }
>>> }
>>> - // If we're done, exit the loop.
>>> - if (IsDone)
>>> - break;
>>> }
>>> }
>>>
>>> Modified: clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h?rev=333993&r1=333992&r2=333993&view=diff
>>>
>>> ==============================================================================
>>> --- clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h (original)
>>> +++ clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h Tue Jun 5
>>> 02:34:46 2018
>>> @@ -102,7 +102,9 @@ enum JSONStreamStyle {
>>> /// if it is.
>>> /// Input stream(\p In) must be opened in binary mode to avoid
>>> preliminary
>>> /// replacements of \r\n with \n.
>>> -void runLanguageServerLoop(std::istream &In, JSONOutput &Out,
>>> +/// We use C-style FILE* for reading as std::istream has unclear
>>> interaction
>>> +/// with signals, which are sent by debuggers on some OSs.
>>> +void runLanguageServerLoop(std::FILE *In, JSONOutput &Out,
>>> JSONStreamStyle InputStyle,
>>> JSONRPCDispatcher &Dispatcher, bool &IsDone);
>>>
>>>
>>> Modified: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp?rev=333993&r1=333992&r2=333993&view=diff
>>>
>>> ==============================================================================
>>> --- clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp (original)
>>> +++ clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp Tue Jun 5
>>> 02:34:46 2018
>>> @@ -238,5 +238,5 @@ int main(int argc, char *argv[]) {
>>> llvm::set_thread_name("clangd.main");
>>> // Change stdin to binary to not lose \r\n on windows.
>>> llvm::sys::ChangeStdinToBinary();
>>> - return LSPServer.run(std::cin, InputStyle) ? 0 :
>>> NoShutdownRequestErrorCode;
>>> + return LSPServer.run(stdin, InputStyle) ? 0 :
>>> NoShutdownRequestErrorCode;
>>> }
>>>
>>> Modified: clang-tools-extra/trunk/test/clangd/too_large.test
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/too_large.test?rev=333993&r1=333992&r2=333993&view=diff
>>>
>>> ==============================================================================
>>> --- clang-tools-extra/trunk/test/clangd/too_large.test (original)
>>> +++ clang-tools-extra/trunk/test/clangd/too_large.test Tue Jun 5
>>> 02:34:46 2018
>>> @@ -4,4 +4,4 @@
>>> #
>>> Content-Length: 2147483648
>>>
>>> -# STDERR: Skipped overly large message
>>> +# STDERR: Refusing to read message
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180608/797a0342/attachment-0001.html>
More information about the cfe-commits
mailing list