This is an archive of the discontinued LLVM Phabricator instance.

Fix the bugs in the mapDiagnosticRanges (Still in progress)
ClosedPublic

Authored by zhengkai on Aug 26 2015, 2:04 PM.

Details

Summary

Use a backtracing alogirthm to match all possible source locations to match the FileID of the Caret location.
This can map the ranges back as much as possible.
You can see some new ranges underlined in the diagnostic information In the test cases modified. This can prove that the new method is effective.
(Solved https://llvm.org/bugs/show_bug.cgi?id=24592)

Also change the standard to judge if the macro backtraces should be reduced.
Using raw encodings to specify if the ranges are all in one argument of the macro invocations.
But this part is still questionable due to some bugs inside the preprocessor system. (See: https://llvm.org/bugs/show_bug.cgi?id=24788)

Diff Detail

Event Timeline

zhengkai updated this revision to Diff 33244.Aug 26 2015, 2:04 PM
zhengkai retitled this revision from to Fix the bugs in the mapDiagnosticRanges (Still in progress).
zhengkai updated this object.
zhengkai added a reviewer: rtrieu.
zhengkai added a subscriber: cfe-commits.
zhengkai updated this revision to Diff 34588.Sep 11 2015, 2:19 PM
zhengkai updated this object.
rtrieu edited edge metadata.Sep 14 2015, 4:59 PM

Why are you leaking the raw encoding of locations from the SourceManager? This is an internal implementation detail and should not be relied on externally. SourceLocation is the typical way to use locations.

Why are you leaking the raw encoding of locations from the SourceManager? This is an internal implementation detail and should not be relied on externally. SourceLocation is the typical way to use locations.

Because I want to check if all the ranges fit in the same argument of one macro invocation, they can be different Source Locations. In ExpansionInfo, these locations have the same ExpansionLocStart.

Why are you leaking the raw encoding of locations from the SourceManager? This is an internal implementation detail and should not be relied on externally. SourceLocation is the typical way to use locations.

Because I want to check if all the ranges fit in the same argument of one macro invocation, they can be different Source Locations. In ExpansionInfo, these locations have the same ExpansionLocStart.

If you call ExpansionInfo::getExpansionLocStart(), then the SourceLocation will be the same if the ExpansionLocStart is the same.

Why are you leaking the raw encoding of locations from the SourceManager? This is an internal implementation detail and should not be relied on externally. SourceLocation is the typical way to use locations.

Because I want to check if all the ranges fit in the same argument of one macro invocation, they can be different Source Locations. In ExpansionInfo, these locations have the same ExpansionLocStart.

If you call ExpansionInfo::getExpansionLocStart(), then the SourceLocation will be the same if the ExpansionLocStart is the same.

But I still need to change the isMacroArg function to get the result of getExpansionLocStart(), and if I want to compare two SourceLocation, I still need to check the raw encoding. I don't know if it is needed not to leak the raw encoding.

Why are you leaking the raw encoding of locations from the SourceManager? This is an internal implementation detail and should not be relied on externally. SourceLocation is the typical way to use locations.

Because I want to check if all the ranges fit in the same argument of one macro invocation, they can be different Source Locations. In ExpansionInfo, these locations have the same ExpansionLocStart.

If you call ExpansionInfo::getExpansionLocStart(), then the SourceLocation will be the same if the ExpansionLocStart is the same.

But I still need to change the isMacroArg function to get the result of getExpansionLocStart(), and if I want to compare two SourceLocation, I still need to check the raw encoding. I don't know if it is needed not to leak the raw encoding.

I kind of see what you are trying to do here, and we should fix it a different way.

  1. Get rid of changes to ExpansionInfo
  2. Change the function SourceManager::isMacroArgExpansion to take two arguments instead of one. The new argument should be a SourceLocation pointer named MacroBegin.
  3. Change the last line of SourceManager::isMacroArgExpansion from "return Expansion.isMacroArgExpansion();" to "if (!Expansion.isMacroArgExpansion()) return false;" After that, add the code to get the macro begin location.
  4. Update DiagnosticRenderer to use SourceLocation instead of raw encodings.
zhengkai updated this revision to Diff 34826.Sep 15 2015, 12:20 PM
zhengkai edited edge metadata.

Use the rtrieu's method to re write the implementation of SourceManager.

Why are you leaking the raw encoding of locations from the SourceManager? This is an internal implementation detail and should not be relied on externally. SourceLocation is the typical way to use locations.

Because I want to check if all the ranges fit in the same argument of one macro invocation, they can be different Source Locations. In ExpansionInfo, these locations have the same ExpansionLocStart.

If you call ExpansionInfo::getExpansionLocStart(), then the SourceLocation will be the same if the ExpansionLocStart is the same.

But I still need to change the isMacroArg function to get the result of getExpansionLocStart(), and if I want to compare two SourceLocation, I still need to check the raw encoding. I don't know if it is needed not to leak the raw encoding.

I kind of see what you are trying to do here, and we should fix it a different way.

  1. Get rid of changes to ExpansionInfo
  2. Change the function SourceManager::isMacroArgExpansion to take two arguments instead of one. The new argument should be a SourceLocation pointer named MacroBegin.
  3. Change the last line of SourceManager::isMacroArgExpansion from "return Expansion.isMacroArgExpansion();" to "if (!Expansion.isMacroArgExpansion()) return false;" After that, add the code to get the macro begin location.
  4. Update DiagnosticRenderer to use SourceLocation instead of raw encodings.

Have changed the implementation.

rtrieu added inline comments.Sep 15 2015, 6:20 PM
include/clang/Basic/SourceManager.h
333

Remove unrelated whitespace change.

1155–1164

Merge the two isMacroArgExpansion functions by making the second argument a SourceLocation pointer with default argument of nullptr. See isAtStartOfImmediateMacroExpansion or isAtEndOfImmediateMacroExpansion functions for how this is done.

lib/Basic/SourceManager.cpp
1015–1024

Merge with other isMacroArgExpansion function

lib/Frontend/DiagnosticRenderer.cpp
311

No newline between comment and function. And what is the logic behind in this function that wasn't in the code it is replacing?

312

Why not return a SourceLocation instead of passing one in by reference?

313

Why is this passed by reference?

314–350

Use more vertical whitespace when coding.

323

In the other function, there is "End = SM->getImmediateExpansionRange(Backup).second;" Why is there not a "Begin = SM->getImmediateExpansionRange(Backup).first;" here?

331

There is a lot of duplicated code between these two functions. Can they merged together with an option to select whether to check the start or end locations?

352–367

Function not used; remove it.

417–418

Can these function be rewritten to return SourceLocation's? Then it would be:
Begin = retrieveBeginLocation(Begin, BeginFileID, CaretLocFileID, SM);
End = retrieveEndLocation(End, EndFileID, CaretLocFileID, SM);

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

466

What is this function doing that is not captured by the old isMacroArgExpansion function?

469

Drop the "= SourceLocation". Default initialization of SourceLocation does the same thing.

471

Use "PreLoc.isInvalid()" instead of "PreLoc == SourceLocation()".

504

Why does the range count matter?

512

Comment is out of date, no longer holding the raw encoding. Rename the variable to ArgumentLoc to convey what the SourceLocation is holding.

513

Drop the "= SourceLocation()". Default initialization for SourceLocation does the same thing.

test/Misc/caret-diags-macros.c
210

Why is Cstrlen changed to strlen_test?

zhengkai updated this revision to Diff 34943.Sep 16 2015, 5:01 PM
zhengkai marked 8 inline comments as done.
zhengkai added inline comments.
lib/Frontend/DiagnosticRenderer.cpp
312

Because this function has two recursive calls inside.
So if return a SourceLocation here, I will need to use a temporal variable to store the result of the first call.
And this doesn't make it easier.

323
Begin = SM->getImmediateMacroCallerLoc(Begin);

Use getImmediateMacroCallerLoc function here.

331

I tried it but the code doesn't seem easier.

352–367

deleted. But I wonder this function is important for those who want to debug this code.

417–418

Replied before.

466

I don't understand what you mean here.

504

Because some ranges passed in are invalid.
And if the mapDiagnosticRanges function drops some ranges, this means this macro call doesn't contain all the information needed of all the ranges.

test/Misc/caret-diags-macros.c
210

The preprocessor can't get right when the argument itself is a function-like macro. This bug is not due to the high-level macro case but due to this problem.

zhengkai marked 2 inline comments as done.Sep 17 2015, 3:49 PM
zhengkai added inline comments.
lib/Frontend/DiagnosticRenderer.cpp
466

Because the function isMacroArgExpansion doesn't check if all locations are in the same argument location,

rtrieu added inline comments.Sep 22 2015, 10:04 PM
lib/Basic/SourceManager.cpp
1008

Fix alignment so function arguments line up.

1012

Add line break after this line.

lib/Frontend/DiagnosticRenderer.cpp
323

You did not add that here.

331

Using reference parameters to input/output data into functions is confusing and generally not done. SourceLocation is a small enough class that it is commonly used as return values and it has a built-in invalid value. Use the following function:

static SourceLocation retrieveMacroLocation(SourceLocation Loc,
                                            FileID MacroFileID,
                                            FileID CaretFileID,
                                            bool getBeginLoc,
                                            const SourceManager *SM) {
  if (MacroFileID == CaretFileID) return Loc;
  if (!Loc.isMacroID()) return SourceLocation();

  SourceLocation MacroLocation, MacroArgLocation;

  if (SM->isMacroArgExpansion(Loc)) {
    MacroLocation = SM->getImmediateMacroCallerLoc(Loc);
    MacroArgLocation = getBeginLoc ? SM->getImmediateExpansionRange(Loc).first
                                   : SM->getImmediateExpansionRange(Loc).second;
  } else {
    MacroLocation = getBeginLoc ? SM->getImmediateExpansionRange(Loc).first
                                : SM->getImmediateExpansionRange(Loc).second;
    MacroArgLocation = SM->getImmediateSpellingLoc(Loc);
  }

  MacroFileID = SM->getFileID(MacroLocation);
  MacroLocation = retrieveMacroLocation(MacroLocation, MacroFileID, CaretFileID,
                                        getBeginLoc, SM);
  if (MacroLocation.isValid()) return MacroLocation;

  MacroFileID = SM->getFileID(MacroArgLocation);
  return retrieveMacroLocation(MacroArgLocation, MacroFileID, CaretFileID,
                               getBeginLoc, SM);
}
352–367

It is more important to have good comments than debug functions since that saves the trouble of running a debugger. If someone is debugging this code, this function would be in the wrong place. Instead, the SourceManager would be a better place to put it, and it saves the trouble of passing in a SourceManager object. Besides that, there special considerations for debugging functions, like annotating it properly so it only shows up in debug builds and not increase the size of regular builds. If you still feel it is important, then the work should go into another patch.

417–418

Use:

Begin = retrieveMacroLocation(Begin, BeginFileID, CaretLocFileID,
                              true /*getBeginLoc*/, SM);
End = retrieveMacroLocation(End, BeginFileID, CaretLocFileID,
                            false /*getBeginLoc*/, SM);
if (Begin.isInvalid() || End.isInvalid()) continue;

with the fixed function above.

466

I still don't know what you are doing here. You have two SourceLocation's, Loc and PreLoc. Those are not good names for telling them apart or what they do. You pass the second SourceLocation in by reference, both as an input value, and later as something to put an output value in. This is a bad idea. If you need a SourceLocation to be passed back to the caller, make the function return a SourceLocation, or use a SourceLocation pointer as an argument that is only for output. The SourceLocation class is tiny so it is okay to copy around.

471

Do the same for NewLoc

482

What is this SourceLocation? Why is it passed by reference?

519

Not done.

test/Misc/caret-diags-macros.c
210

Add a FIXME comment that it should be changed back to Cstrlen when the bug is resolved.

zhengkai updated this revision to Diff 35554.Sep 23 2015, 2:07 PM
zhengkai marked 11 inline comments as done.
zhengkai added inline comments.
lib/Frontend/DiagnosticRenderer.cpp
323
The getImmediateMacroCallerLoc is implemented as following:

SourceLocation getImmediateMacroCallerLoc(SourceLocation Loc) const {
  if (!Loc.isMacroID()) return Loc;
  if (isMacroArgExpansion(Loc))
    return getImmediateSpellingLoc(Loc);

  return getImmediateExpansionRange(Loc).first;
}
482

Changed

rtrieu added inline comments.Sep 23 2015, 2:51 PM
lib/Frontend/DiagnosticRenderer.cpp
466

Still not answered: What is PreLoc? Why is it important to this function? What does PreLoc even mean?

482

What is Loc? Where is it pointing to? Why do you need it?

test/Misc/caret-diags-macros.c
210

Use:

// FIXME: Change test to use 'Cstrlen' instead of 'strlen_test' when macro printing is fixed.
zhengkai updated this revision to Diff 35556.Sep 23 2015, 2:57 PM
zhengkai marked an inline comment as done.
zhengkai added inline comments.
lib/Frontend/DiagnosticRenderer.cpp
466

I have stated that I need to check if all the locations checked fit in the same location in the expansionInfo. So I need the preLoc to store the previous checked location to see if they are the same.

482

See the comment ahead.

rtrieu added inline comments.Sep 23 2015, 3:14 PM
lib/Frontend/DiagnosticRenderer.cpp
466

That explains nothing. You have a function 'checkLocForMacroArgExpansion' that has three parameters, SourceLocation Loc, SourceManager SM, and SourceLocation PreLoc. From the name of the function, it appears that it checks Loc to see if is a macro argument expansion. The SourceManger is needed for checking information about SourceLocation's. That leaves PreLoc, which does not have a descriptive name ('pre' could be short for lots of different words) nor is it commented.

You mentioned it is the previously checked location. Does this mean PreLoc is one location behind Loc? If so, why not just use the offset to get one behind? Or does it refer to different locations on the macro stack?

Since both 'checkLocForMacroArgExpansion' and 'checkRangeForMacroArgExpansion' now have a third parameter that is not obvious from the function name what they do, add comments to the functions to explain the purpose of the third parameter.

zhengkai updated this revision to Diff 35561.Sep 23 2015, 3:22 PM
zhengkai added inline comments.
lib/Frontend/DiagnosticRenderer.cpp
466

Explained in the new comments in the updated diff.

zhengkai updated this revision to Diff 35573.Sep 23 2015, 4:47 PM
zhengkai updated this revision to Diff 35574.
zhengkai updated this revision to Diff 35575.Sep 23 2015, 5:06 PM
This revision was automatically updated to reflect the committed changes.