[clang][Frontend] Fix a crash in DiagnosticRenderer.

[balazske]

Summary

Displaying the problem range could crash if the begin and end of a
range is in different files or macros. After the change such range
is displayed only as the beginning location.

There is a bug for this problem:
https://bugs.llvm.org/show_bug.cgi?id=46540

[steakhal]

Please, also add a regression test for Haoxin Tu's reproducer.

// RUN: %clang %s

// expected warning....
volatile long a ( a .~b

[balazske]

In D95860#2536609, @steakhal wrote:

Please, also add a regression test for Haoxin Tu's reproducer.

// RUN: %clang %s

// expected warning....
volatile long a ( a .~b

What about the already added test?

[steakhal]

In D95860#2536721, @balazske wrote:

In D95860#2536609, @steakhal wrote:

Please, also add a regression test for Haoxin Tu's reproducer.

// RUN: %clang %s

// expected warning....
volatile long a ( a .~b

What about the already added test?

Oh sorry, I was tricked by that the analyzer is enabled.

We would crash even without the analyzer, so pushing this test under the Analysis seems unnecessary to me.
BTW, the analysis expects a compiler-error-free program. This shouldn't compile. This is not related to analysis in any way.

The alpha.clone.CloneChecker reproducer however, crashes at the end of the analysis when it tries to emit the diagnostics. IMO that should be in the Analysis, the current one should be where frontend tests are.
IMO we should have tests for both of these.

[balazske]

Existing test moved to Frontend, added new test.

[balazske]

Reformatting test file.

[steakhal]

In D95860#2538802, @balazske wrote:

Existing test moved to Frontend, added new test.

Awesome, thanks!

On the functional side of things, I'm not sure if we should skip or not.
But simply skipping those seems like you are treating the symptom, not the cause.
Could you please give a rationale why can't we implement the subsequent logic in a way that it does not require BeginFileID == EndFileID to hold?

[balazske]

I realized that the added test cases show different problems: One is when one of Begin or End is invalid, other if the FileID's are different. Probably we can find a better solution for the second problem. So the patch needs to be splitted and the case with the crash-diagnostic-renderer.cpp goes into the first part.

[balazske]

But I do not know if there is a better solution, if the file ID is different these are in different files, one included from the other. If the "include locations" are not accessible in the same way as macro locations we can not get the place (of the #include statement) in the original file. Additionally this may be not always the best thing to display to the user.

[balazske]

Probably it is not worth to find a better solution. In the case of CloneChecker the range starts and ends on different line that is not possible to print on a single line in the output.
This code could work:

// Then, crawl the expansion chain for the end of the range.
if (BeginFileID != EndFileID) {
  while (End.isMacroID() && !BeginLocsMap.count(EndFileID)) {
    auto Exp = SM->getImmediateExpansionRange(End);
    IsTokenRange = Exp.isTokenRange();
    End = Exp.getEnd();
    EndFileID = SM->getFileID(End);
  }
  if (End.isMacroID()) {
    Begin = BeginLocsMap[EndFileID];
    BeginFileID = EndFileID;
  } else if (BeginFileID != EndFileID) {
    assert(!BeginLocsMap.count(EndFileID));
    //if (SM->getIncludeLoc(BeginFileID).isValid() && SM->getIncludeLoc(EndFileID).isValid())
    //  continue;
    if (SM->getIncludeLoc(BeginFileID).isValid()) {
      while (Begin.isValid() && BeginFileID != EndFileID) {
        Begin = SM->getIncludeLoc(BeginFileID);
        BeginFileID = SM->getFileID(Begin);
      }
    } else if (SM->getIncludeLoc(EndFileID).isValid()) {
      while (End.isValid() && BeginFileID != EndFileID) {
        End = SM->getIncludeLoc(EndFileID);
        EndFileID = SM->getFileID(End);
      }
    } else {
      llvm_unreachable("Got unacceptable source range with begin and end in different translation unit?");
    }
  }
}

if (Begin.isInvalid() || End.isInvalid())
  continue;

// Do the backtracking.

With this solution the following result is printed:

llvm-project/clang/test/Analysis/copypaste/clone-end-in-other-file.cpp:9:3: warning: Duplicate code detected [alpha.clone.CloneChecker]
  if (i == 10) // expected-warning{{Duplicate code detected}}
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
llvm-project/clang/test/Analysis/copypaste/clone-end-in-other-file.cpp:11:3: note: Similar code here
  if (i == 10) // expected-note{{Similar code here}}
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

This is not better than the result with the existing code:

llvm-project/clang/test/Analysis/copypaste/clone-end-in-other-file.cpp:9:3: warning: Duplicate code detected [alpha.clone.CloneChecker]
  if (i == 10) // expected-warning{{Duplicate code detected}}
  ^
llvm-project/clang/test/Analysis/copypaste/clone-end-in-other-file.cpp:11:3: note: Similar code here
  if (i == 10) // expected-note{{Similar code here}}
  ^
1 warning generated.

[steakhal]

In D95860#2539453, @balazske wrote:

Probably it is not worth to find a better solution. In the case of CloneChecker the range starts and ends on different line that is not possible to print on a single line in the output.
This code could work:
[code...]
With this solution the following result is printed:

llvm-project/clang/test/Analysis/copypaste/clone-end-in-other-file.cpp:9:3: warning: Duplicate code detected [alpha.clone.CloneChecker]
  if (i == 10) // expected-warning{{Duplicate code detected}}
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
llvm-project/clang/test/Analysis/copypaste/clone-end-in-other-file.cpp:11:3: note: Similar code here
  if (i == 10) // expected-note{{Similar code here}}
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

This is not better than the result with the existing code:

llvm-project/clang/test/Analysis/copypaste/clone-end-in-other-file.cpp:9:3: warning: Duplicate code detected [alpha.clone.CloneChecker]
  if (i == 10) // expected-warning{{Duplicate code detected}}
  ^
llvm-project/clang/test/Analysis/copypaste/clone-end-in-other-file.cpp:11:3: note: Similar code here
  if (i == 10) // expected-note{{Similar code here}}
  ^
1 warning generated.

I think you are right. There is no gain in complicating this. I don't think there is anything that we can do about this.
Your original patch, simply ignoring the end if that is invalid or from a different file is fine by me.

Thank you for going the extra mile!

In D95860#2538845, @balazske wrote:

I realized that the added test cases show different problems: One is when one of Begin or End is invalid, other if the FileID's are different. Probably we can find a better solution for the second problem. So the patch needs to be splitted and the case with the crash-diagnostic-renderer.cpp goes into the first part.

I don't mind committing this in one go.
However, whichever you choose, please update the revision summary according to the commit message.
You should definitely describe both of these issues you found.

Thanks for your effort.

[balazske]

Added explanation to source code.
Added more test cases.

clang/lib/Frontend/DiagnosticRenderer.cpp

if (BeginFileID != EndFileID) {
         EndFileID = SM->getFileID(End);
       }
       if (End.isMacroID()) {
         Begin = BeginLocsMap[EndFileID];
         BeginFileID = EndFileID;
       }
     }
 
+    if (Begin.isInvalid() || End.isInvalid() || BeginFileID != EndFileID)
+      continue;
+
     // Do the backtracking.
     SmallVector<FileID, 4> CommonArgExpansions;
     computeCommonMacroArgExpansionFileIDs(Begin, End, SM, CommonArgExpansions);
     Begin = retrieveMacroLocation(Begin, BeginFileID, CaretLocFileID,
                                   CommonArgExpansions, /*IsBegin=*/true, SM,
                                   IsTokenRange);
     End = retrieveMacroLocation(End, BeginFileID, CaretLocFileID,
                                 CommonArgExpansions, /*IsBegin=*/false, SM,

clang/test/Analysis/crash-diagnostic-renderer.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+// This case reproduces a problem that is shown here:
+// https://bugs.llvm.org/show_bug.cgi?id=46540
+// No assertion should happen in this case.
+
+// expected-error@13{{'b' does not refer to a type name in pseudo-destructor expression; expected the name of type 'volatile long'}}
+// expected-error@13{{expected ')'}}
+// expected-note@13{{to match this '('}}
+// expected-error@13{{reference to pseudo-destructor must be called; did you mean to call it with no arguments?}}
+// expected-error@13{{cannot initialize a variable of type 'volatile long' with an rvalue of type 'void'}}
+// expected-error@13{{expected ';' after top level declarator}}
+volatile long a ( a .~b