[clang-format] Remove the dependency on frontend

[MyDeveloperDay]

Summary

Address review comments from D68554: [clang-format] Proposal for clang-format to give compiler style warnings by trying to drop the dependency again on Frontend whilst keeping the same format diagnostic messages

Not completely happy with having to do a split in order to get the StringRef for the Line the error occurred on, but could see a way to use SourceManager and SourceLocation to give me a single line?

But this removes the dependency on frontend which should keep the binary size down.

[klimek]

My intuitive solution would have been to get the char* for the start and end-location and then search forward and backwards for \n.

[MyDeveloperDay]

In D68969#1709321, @klimek wrote:

My intuitive solution would have been to get the char* for the start and end-location and then search forward and backwards for \n.

I may need to, it feels slow for large files which I suspect is in the splitting, which will be mostly unnecessary.

[klimek]

In D68969#1709696, @MyDeveloperDay wrote:

In D68969#1709321, @klimek wrote:

My intuitive solution would have been to get the char* for the start and end-location and then search forward and backwards for \n.

I may need to, it feels slow for large files which I suspect is in the splitting, which will be mostly unnecessary.

It's a weird trade-off: for lots of lines, the inside-out algorithm will be better, but if we have a very very long line with lots of warnings, the inside-out algorithm will be quadratic.

[MyDeveloperDay]

Remove need to split lines or search for \n

General Algorithm

1. take Location of Replacement line and column
2. make a new SourceLocation of line and column =0 (beginning of that replacement line)
3. make a new SourceLocation of line+1 and column = 0 (beginning of the next line)
4. use getCharacterData() to get pointers to beginning of each line
5. make a String from those begin and end ptrs

Seems very much faster when run over clang source tree (removal of noticeable pauses on large files)

[klimek]

clang/tools/clang-format/ClangFormat.cpp

344

Looks unused?

350

This is unexpected: I'd have expected this to be PLoc.getLine() here and PLoc.getLine() + 1 below. I'm probably missing something?

[MyDeveloperDay]

clang/tools/clang-format/ClangFormat.cpp

344

correct, I changed my mind and now call ArrayRef<std::pair<unsigned, unsigned>>() as the last argument not sure if thay would be slower than me creating one empty one outside the loop, any thoughts? I'll go with a consensus and will remove the other.

350

This caught me out too, but it kept pulling the next line. So I think it must be a zero based line as opposed to PesumedLoc which uses line 1 to mean the first line..

but leet me double check.

[MyDeveloperDay]

clang/tools/clang-format/ClangFormat.cpp

350

OK this then perhaps is wrong, given the assert.. let me double check what is going on.

SourceLocation SourceManager::translateFileLineCol(const FileEntry *SourceFile,
                                                  unsigned Line,
                                                  unsigned Col) const {
  assert(SourceFile && "Null source file!");
  assert(Line && Col && "Line and column should start from 1!");

  FileID FirstFID = translateFile(SourceFile);
  return translateLineCol(FirstFID, Line, Col);
}

[MyDeveloperDay]

Update out by one errors and unused variables

[thakis]

klimek: ping :)

[klimek]

lg

clang/tools/clang-format/ClangFormat.cpp

357

• 1 is to exclude the \n I'd assume?

[MyDeveloperDay]

clang/tools/clang-format/ClangFormat.cpp

357

correct, without the -1 I end up with an extra blank line in the output

[thakis]

Since this was reverted: are you looking into relanding this?

[MyDeveloperDay]

In D68969#1732489, @thakis wrote:

Since this was reverted: are you looking into relanding this?

I'm not totally sure how they reproduced the UBSAN issue

[thakis]

The revert message said:

Vlad Tsyrklevich via cfe-commits <[e-mail]>
Tue, Oct 29, 1:51 PM (7 days ago)
to via, mydeveloperday

I've reverted this commit as it was causing UBSan failures on the ubsan bot. These failures looked like:
llvm/lib/Support/SourceMgr.cpp:440:48: runtime error: pointer index expression with base 0x000000000000 overflowed to 0xfffffffffffffffa

Looking at a backtrace, this line was reached from the `Diags.print(nullptr, llvm::errs(), (ShowColors && !NoShowColors));` call introduced in this change.

Is that enough to see what's wrong? If not, @vlad.tsyrklevich , can you give repro steps for that ubsan failure that made you revert ec66603 in efed314?

[MyDeveloperDay]

In D68969#1733946, @thakis wrote:

The revert message said:

Vlad Tsyrklevich via cfe-commits <[e-mail]>
Tue, Oct 29, 1:51 PM (7 days ago)
to via, mydeveloperday

I've reverted this commit as it was causing UBSan failures on the ubsan bot. These failures looked like:
llvm/lib/Support/SourceMgr.cpp:440:48: runtime error: pointer index expression with base 0x000000000000 overflowed to 0xfffffffffffffffa

Looking at a backtrace, this line was reached from the `Diags.print(nullptr, llvm::errs(), (ShowColors && !NoShowColors));` call introduced in this change.

Is that enough to see what's wrong? If not, @vlad.tsyrklevich , can you give repro steps for that ubsan failure that made you revert ec66603 in efed314?

Note to self: Took me a bit to set up a docker container to build with clang-10 on linux (I'm normally a windows guy), but hopefully, I can debug from here...

test1.cpp:1:7: warning: code should be clang-formatted [-Wclang-format-violations]
/llvm-project/llvm/lib/Support/SourceMgr.cpp:440:48: runtime error: applying non-zero offset 1844674407
3709551610 to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /buildareas/llvm2/llvm-project/llvm/lib/Support/SourceMgr.cpp:44
0:48 in

[MyDeveloperDay]

see D69854: [clang-format] [RELAND] Remove the dependency on frontend

clang/tools/clang-format/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_tool(clang-format
   ClangFormat.cpp
   )
 
 set(CLANG_FORMAT_LIB_DEPS
   clangBasic
   clangFormat
-  clangFrontend
   clangRewrite
   clangToolingCore
   )
 
 clang_target_link_libraries(clang-format
   PRIVATE
   ${CLANG_FORMAT_LIB_DEPS}
   )

clang/tools/clang-format/ClangFormat.cpp

 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Version.h"
 #include "clang/Format/Format.h"
-#include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Process.h"
 
 using namespace llvm;
 using clang::tooling::Replacements;
emitReplacementWarnings(const Replacements &Replaces, StringRef AssumedFileName,
                         const std::unique_ptr<llvm::MemoryBuffer> &Code) {
   if (Replaces.empty()) {
     return false;
   }
 
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
   DiagOpts->ShowColors = (ShowColors && !NoShowColors);
 
-  TextDiagnosticPrinter *DiagsBuffer =
-      new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts, false);
-
   IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
   IntrusiveRefCntPtr<DiagnosticsEngine> Diags(
-      new DiagnosticsEngine(DiagID, &*DiagOpts, DiagsBuffer));
+      new DiagnosticsEngine(DiagID, &*DiagOpts));
 
   IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFileSystem(
       new llvm::vfs::InMemoryFileSystem);
   FileManager Files(FileSystemOptions(), InMemoryFileSystem);
   SourceManager Sources(*Diags, Files);
   FileID FileID = createInMemoryFile(AssumedFileName, Code.get(), Sources,
                                      Files, InMemoryFileSystem.get());
 
-  const unsigned ID = Diags->getCustomDiagID(
-      WarningsAsErrors ? clang::DiagnosticsEngine::Error
-                       : clang::DiagnosticsEngine::Warning,
-      "code should be clang-formatted [-Wclang-format-violations]");
+  SmallVector<StringRef, 8> Lines;
+  Code->getBuffer().split(Lines, "\n", /*MaxSplit=*/-1,
+                          /*KeepEmpty=*/true);
 
   unsigned Errors = 0;
-  DiagsBuffer->BeginSourceFile(LangOptions(), nullptr);
   if (WarnFormat && !NoWarnFormat) {
+    ArrayRef<std::pair<unsigned, unsigned>> Ranges;
     for (const auto &R : Replaces) {
-      Diags->Report(
-          Sources.getLocForStartOfFile(FileID).getLocWithOffset(R.getOffset()),
-          ID);
+      PresumedLoc PLoc = Sources.getPresumedLoc(
+          Sources.getLocForStartOfFile(FileID).getLocWithOffset(R.getOffset()));
+
+      StringRef Line;
+      if (PLoc.getLine() < Lines.size() && PLoc.getLine() > 0)
+        Line = Lines[PLoc.getLine() - 1];
+
+      SMDiagnostic Diags(
+          llvm::SourceMgr(), SMLoc(), AssumedFileName, PLoc.getLine(),
+          PLoc.getColumn() - 1,
+          WarningsAsErrors ? SourceMgr::DiagKind::DK_Error
+                           : SourceMgr::DiagKind::DK_Warning,
+          "code should be clang-formatted [-Wclang-format-violations]", Line,
+          ArrayRef<std::pair<unsigned, unsigned>>());
+
+      Diags.print(nullptr, llvm::errs(), (ShowColors && !NoShowColors));
       Errors++;
       if (ErrorLimit && Errors >= ErrorLimit)
         break;
     }
   }
-  DiagsBuffer->EndSourceFile();
   return WarningsAsErrors;
 }
 
 static void outputXML(const Replacements &Replaces,
                       const Replacements &FormatChanges,
                       const FormattingAttemptStatus &Status,
                       const cl::opt<unsigned> &Cursor,
                       unsigned CursorPosition) {