thanks! On Fri, Jun 8, 2018 at 1:31 PM Sam McCall <sam.mcc...@gmail.com> wrote:
> Oops, thank you! > r334315 should fix this. > > > > On Fri, Jun 8, 2018 at 9:45 PM Kostya Serebryany <k...@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@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@lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
