[PATCH] D137751: Produce a more informative diagnostics when Clang runs out of source locations

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D137751#3919162 <https://reviews.llvm.org/D137751#3919162>, @alexfh wrote:

> I wonder whether we can include SLoc usage information into `-print-stats` output?

Yeah, we could extend the source manager information there with something like this. It's a bit awkward to reuse this implementation because it's geared around producing notes, but it should be fairly straightforward to factor out some common code and extend `-print-stats` with it in a follow-up change.



================
Comment at: clang/include/clang/Basic/SourceManager.h:1695-1696
+  // Produce notes describing the current source location address space usage.
+  void noteSLocAddressSpaceUsage(DiagnosticsEngine &Diag,
+                                 unsigned MaxNotes = 50) const;
+
----------------
aaron.ballman wrote:
> Not that I'm opposed, but how did you arrive at 50?
I was aiming for a number that's small enough that the diagnostic won't run for pages (will fit into a relatively small scroll buffer) but large enough that it should capture problematic usage.

But, you know what, we should be consistent. A default of 32 would match our default limit for notes in constexpr and template backtraces; let's use that. Plus powers of 2 are the best arbitrary numbers.


================
Comment at: clang/lib/Basic/SourceManager.cpp:675
   LocalSLocEntryTable.push_back(SLocEntry::get(NextLocalOffset, Info));
+  // TODO: Produce a proper diagnostic for this case.
   assert(NextLocalOffset + Length + 1 > NextLocalOffset &&
----------------
aaron.ballman wrote:
> Are you planning to do this as part of this patch, or is this more of an aspirational FIXME for the future?
This is aspirational. I think it'll be a lot of work to make all the transitive callers of this be able to handle it failing.


================
Comment at: clang/lib/Basic/SourceManager.cpp:2251
+  uint64_t CountedSize = 0;
+  for (int IDIndex = -(int)LoadedSLocEntryTable.size() - 1;
+       IDIndex < (int)LocalSLocEntryTable.size(); ++IDIndex) {
----------------
shafik wrote:
> alexfh wrote:
> > Could you add a comment about the meaning of negative IDIndex values?
> Maybe make `-(int)LoadedSLocEntryTable.size()` a named variable may help readability as well.  Perhaps also for `(int)LocalSLocEntryTable.size()` as well.
Refactored to make it clearer what's going on, added comments.


================
Comment at: clang/lib/Basic/SourceManager.cpp:2281-2282
+  SortedUsage.reserve(Usage.size());
+  for (auto It = Usage.begin(); It != Usage.end(); ++It)
+    SortedUsage.push_back(It);
+  auto Cmp = [](UsageMap::iterator A, UsageMap::iterator B) {
----------------
aaron.ballman wrote:
> llvm::copy?
This is forming a vector of iterators, not a vector of values. Hm, but... I think I can express this better with `MapVector` and avoid this copy in the process.


================
Comment at: clang/lib/Lex/Pragma.cpp:1187
+      // specifically report information about.
+      uint64_t MaxNotes = (uint64_t)-1;
+      Token ArgToken;
----------------
aaron.ballman wrote:
> `~0ULL`?
That would be `(unsigned long long)-1`, which isn't necessarily the same value.

But I think we can do better than forming a large unsigned number here; changed to use `Optional`.


================
Comment at: clang/test/Misc/sloc-usage.cpp:1
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
----------------
aaron.ballman wrote:
> Should we do anything special about source locations from the command line, or do those not contribute source locations? (e.g., `-D` or `-U`, force include file, etc flags)
> 
> (Testing a forced include would be interesting regardless, just to ensure we catch those the same as we do an `#include`.)
For `-D` etc, we get output that looks like this:

```
<built-in>:1:1: note: file entered 2 times using 14778B of space
# 1 "<built-in>" 3
^
```

... where the `<built-in>:1:1` is the location of the first time we enter a `FileID` with a null corresponding `const FileEntry*`. I think these will all correspond to predefines and similar things, so the `<built-in>:1:1` source location seems pretty reasonable.

I've added a test for `-include`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137751/new/

https://reviews.llvm.org/D137751