[clang-format-diff] Fix missing formatting for zero length git diff lines

[zequanwu]

Summary

If we only delete lines that are outer block statements (if, while, etc),
clang-format-diff.py can't format the statements inside the block statements.

An example to repro:

1. Delete the if statment at line 118 in llvm/lib/CodeGen/Analysis.cpp.
2. Run git diff -U0 --no-color HEAD^ | clang/tools/clang-format/clang-format-diff.py -i -p1

It fails to format the statement after if.

[hans]

Are there no tests for clang-format-diff.py? That seems unfortunate.

Will this handle more nested cases, for example if the first line here is deleted, will we fix the indent of both "if (b)" and "c"?

if (a)
  if (b)
    c;

I worry that this might be a tricky problem..

[zequanwu]

In D111273#3047759, @hans wrote:

Are there no tests for clang-format-diff.py? That seems unfortunate.

No test for clang-format-diff.py.

Will this handle more nested cases, for example if the first line here is deleted, will we fix the indent of both "if (b)" and "c"?

if (a)
  if (b)
    c;

I worry that this might be a tricky problem..

It handles this. Because git diff -U0 gives 0 context lines, deleting lines will always gives 0 line_count.

[hans]

lgtm

If you want to look into it, I think having some kind of test for this script would be useful.

[MyDeveloperDay]

So this isn't just clang-format-diff.py but also all of git clang-format

<edit the file>
git add Analysis.cpp
$ git clang-format
clang-format did not modify any files

This is bad because this is a way in which those using "git clang-format" can miss formatting a line

So ultimately looking at the diff (normal diff)

$ git diff --cached Analysis.cpp
diff --git a/llvm/lib/CodeGen/Analysis.cpp b/llvm/lib/CodeGen/Analysis.cpp
index e5d576d879b5..3107fc0b9936 100644
--- a/llvm/lib/CodeGen/Analysis.cpp
+++ b/llvm/lib/CodeGen/Analysis.cpp
@@ -115,7 +115,6 @@ void llvm::ComputeValueVTs(const TargetLowering &TLI, const DataLayout &DL,
     return;
   // Base case: we can get an EVT for this LLVM IR type.
   ValueVTs.push_back(TLI.getValueType(DL, Ty));
-  if (MemVTs)
     MemVTs->push_back(TLI.getMemValueType(DL, Ty));
   if (Offsets)
     Offsets->push_back(StartingOffset);

This is going to give us a start_line of 115 and a line_count of 6 and hence an end_line of 115+5 = 120

if clang-format is given those lines I WOULD expect it to continue and correct it.

$ clang-format -n --lines 115:120 Analysis.cpp
Analysis.cpp:117:48: warning: code should be clang-formatted [-Wclang-format-violations]
  ValueVTs.push_back(TLI.getValueType(DL, Ty));

But like clang-format-diff here, git-clang-format is doing a -U0 diff hence the line count is 0

$ git diff -U0 --cached Analysis.cpp
diff --git a/llvm/lib/CodeGen/Analysis.cpp b/llvm/lib/CodeGen/Analysis.cpp
index e5d576d879b5..3107fc0b9936 100644
--- a/llvm/lib/CodeGen/Analysis.cpp
+++ b/llvm/lib/CodeGen/Analysis.cpp
@@ -118 +117,0 @@ void llvm::ComputeValueVTs(const TargetLowering &TLI, const DataLayout &DL,
-  if (MemVTs)

git-clang-format would also not process that change (as line_count == 0)

The code for handling this in git-clang-format is subtly different but equally potentially wrong

def extract_lines(patch_file):
  """Extract the changed lines in `patch_file`.

  The return value is a dictionary mapping filename to a list of (start_line,
  line_count) pairs.

  The input must have been produced with ``-U0``, meaning unidiff format with
  zero lines of context.  The return value is a dict mapping filename to a
  list of line `Range`s."""
  matches = {}
  for line in patch_file:
    line = convert_string(line)
    match = re.search(r'^\+\+\+\ [^/]+/(.*)', line)
    if match:
      filename = match.group(1).rstrip('\r\n')
    match = re.search(r'^@@ -[0-9,]+ \+(\d+)(,(\d+))?', line)
    if match:
      start_line = int(match.group(1))
      line_count = 1
      if match.group(3):
        line_count = int(match.group(3))
      if line_count > 0:
        matches.setdefault(filename, []).append(Range(start_line, line_count))
  return matches

However if we take the same approach as suggested here, clang-format will run over that line

$ clang-format -n --lines 117:117 Analysis.cpp
Analysis.cpp:117:48: warning: code should be clang-formatted [-Wclang-format-violations]
  ValueVTs.push_back(TLI.getValueType(DL, Ty));

To answer @hans question, I think it will do the correct thing, clang-format still works on the whole file, it just filters the replacements based on the "line range" given by --lines so I would expect it to work

I think this is the correct change being proposed here, but I also think git-clang-format is wrong.

[zequanwu]

Fix the same error in git-clang-format.

[lhames]

Heads up -- I think I just hit an error due to this while formatting a local commit:

% ./clang/tools/clang-format/git-clang-format HEAD~1                                                                                                   
Assertion failed: (Line && Col && "Line and column should start from 1!"), function translateLineCol, file ./llvm-project/clang/lib/Basic/SourceManager.cpp, line 1699.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.            
Stack dump:                                                                                                    
0.      Program arguments: clang-format -lines=15:15 -lines=24:24 -lines=46:47 -lines=73:74 -lines=82:82 -lines=117:117 -lines=128:128 -lines=168:168 -lines=177:177 -lines=0:0 llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):                                                                               
0  clang-format             0x0000000101390dad llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 61
1  clang-format             0x000000010139135b PrintStackTraceSignalHandler(void*) + 27
2  clang-format             0x000000010138f00b llvm::sys::RunSignalHandlers() + 123                            
3  clang-format             0x0000000101393fb8 SignalHandler(int) + 232                                        
4  libsystem_platform.dylib 0x00007fff20639d7d _sigtramp + 29                                                  
5  libsystem_platform.dylib 000000000000000000 _sigtramp + 18446603339972764320
6  libsystem_c.dylib        0x00007fff20549411 abort + 120                                                                                                                                                                     
7  libsystem_c.dylib        0x00007fff205487e8 err + 0
8  clang-format             0x000000010146e898 clang::SourceManager::translateLineCol(clang::FileID, unsigned int, unsigned int) const + 136
9  clang-format             0x00000001011eb586 clang::format::fillRanges(llvm::MemoryBuffer*, std::__1::vector<clang::tooling::Range, std::__1::allocator<clang::tooling::Range> >&) + 966                                     
10 clang-format             0x00000001011dd718 clang::format::format(llvm::StringRef) + 1064                   
11 clang-format             0x00000001011dcc1b main + 955                                                                                                                                                                      
12 libdyld.dylib            0x00007fff2060ff3d start + 1                                                       
13 libdyld.dylib            0x000000000000000c start + 18446603339972935888                                                                                                                                                    
error: `clang-format -lines=15:15 -lines=24:24 -lines=46:47 -lines=73:74 -lines=82:82 -lines=117:117 -lines=128:128 -lines=168:168 -lines=177:177 -lines=0:0 llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp` failed

Reverting this patch fixed the error.

I am able to reliably reproduce this crash on Darwin by running:

% git checkout fd26ca4e7515e7dd32ae02e777bd21693afc68ff
% git am jitlink-table-manager-updates.patch
% ./clang/tools/clang-format/git-clang-format HEAD~1

jitlink-table-manager-updates.patch5 KBDownload

[MyDeveloperDay]

This bug only seems to impact git-clang-format and not git-format-diff.py

The diff that git-clang-format will run is this...

git diff-index -p -U0 HEAD~ --

and it should produce the diffbelow, I assume because you are deleting TableManager.h

its this

@@ -1,75 +0,0 @@

That is causing the 0 startline and 0 line diff (as you might expect there is nothing to clang-format.

I think the simplest fix is to guard against startline being 0

diff --git a/llvm/lib/ExecutionEngine/JITLink/TableManager.h b/llvm/lib/ExecutionEngine/JITLink/TableManager.h
deleted file mode 100644
index e03948b49b21..000000000000
--- a/llvm/lib/ExecutionEngine/JITLink/TableManager.h
+++ /dev/null
@@ -1,75 +0,0 @@
-//===---------------------- TableManager.h ----------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
....

[MyDeveloperDay]

The following fix should resolve this

$ git diff git-clang-format
diff --git a/clang/tools/clang-format/git-clang-format b/clang/tools/clang-format/git-clang-format
index 40e848d55a8c..c7e15eb7b483 100755
--- a/clang/tools/clang-format/git-clang-format
+++ b/clang/tools/clang-format/git-clang-format
@@ -327,6 +327,8 @@ def extract_lines(patch_file):
         line_count = int(match.group(3))
       if line_count == 0:
         line_count = 1
+      if start_line == 0:
+        continue
       matches.setdefault(filename, []).append(Range(start_line, line_count))
   return matches

$ ./clang/tools/clang-format/git-clang-format -v -v HEAD~1
Ignoring changes in the following files (wrong extension or symlink):
    clang/tools/clang-format/git-clang-format
Running clang-format on the following files:
    llvm/include/llvm/ExecutionEngine/JITLink/TableManager.h
    llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
old tree: 7584627b571deb7ed22068eb6e0496b51d76b63f
new tree: e88c2fb484c4fd0d63e150fa8d666c794ffd18ce
changed files:
    llvm/include/llvm/ExecutionEngine/JITLink/TableManager.h

clang/tools/clang-format/clang-format-diff.py

else:
         continue
 
     match = re.search(r'^@@.*\+(\d+)(,(\d+))?', line)
     if match:
       start_line = int(match.group(1))
       line_count = 1
       if match.group(3):
         line_count = int(match.group(3))
-      if line_count == 0:
-        continue
-      end_line = start_line + line_count - 1
+      # Also format lines range if line_count is 0 in case of deleting
+      # surrounding statements.
+      end_line = start_line
+      if line_count != 0:
+        end_line += line_count - 1
       lines_by_file.setdefault(filename, []).extend(
           ['-lines', str(start_line) + ':' + str(end_line)])
 
   # Reformat files containing changes in place.
   for filename, lines in lines_by_file.items():
     if args.i and args.verbose:
       print('Formatting {}'.format(filename))
     command = [args.binary, filename]

clang/tools/clang-format/git-clang-format

for line in patch_file:
     if match:
       filename = match.group(1).rstrip('\r\n')
     match = re.search(r'^@@ -[0-9,]+ \+(\d+)(,(\d+))?', line)
     if match:
       start_line = int(match.group(1))
       line_count = 1
       if match.group(3):
         line_count = int(match.group(3))
-      if line_count > 0:
-        matches.setdefault(filename, []).append(Range(start_line, line_count))
+      if line_count == 0:
+        line_count = 1
   return matches
 
 
 def filter_by_extension(dictionary, allowed_extensions):
   """Delete every key in `dictionary` that doesn't have an allowed extension.
 
   `allowed_extensions` must be a collection of lowercase file extensions,
   excluding the period."""