[clang] Make negative getLocWithOffset widening-safe.

[simon_tatham]

Summary

This is part of a patch series working towards the ability to make
SourceLocation into a 64-bit type to handle larger translation units.

In many places in the code, SourceLocation::getLocWithOffset is used
with an argument of type unsigned, holding a value that's
conceptually negative (to find a location shortly before the current
one). If SourceLocation becomes a 64-bit type, this won't work any
more, because the negation will happen before widening to 64 bits, so
that instead of an offset of (say) -3, you'll get 2^32-3.

This patch fixes that problem in the short term, by adding casts to
SourceLocation::IntType at all the current call sites. I haven't
thought of a reliable way to protect against the introduction of new
call sites that similarly get it wrong, though.

[tmatheson]

I'm not a huge fan of this as an API; it is not obvious what the function and parameters do without reading the comment or implementation (e.g. whether Offset is ignored if NegOffset is given, and what it means to pass offsets to both). It moves the subtraction logic from the call site, where it is meaningful, to inside the function where it is not. I also think it's the caller's responsibility to make sure what they pass in is correctly signed. I've put suggestions for how each call site could be updated without casting while keeping getLocWithOffset unchanged.

clang/lib/AST/SelectorLocationsKind.cpp

41

These are probably fine as they were

clang/lib/CodeGen/CoverageMappingGen.cpp

235–238

Seems like getFileOffset should now return ui64.

clang/lib/Format/FormatTokenLexer.cpp

835–836

This probably would need a cast; widening TrailingWhitespace seems overkill.

clang/lib/Lex/Lexer.cpp

535

ditto getDecomposedLoc

568–570

Shouldn't the return type of getDecomposedLoc need updating at some stage? If so that would take care of this case

clang/lib/Parse/ParseStmtAsm.cpp

174–179

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

1073–1074

[simon_tatham]

In D105495#2882628, @tmatheson wrote:

I also think it's the caller's responsibility to make sure what they pass in is correctly signed.

I don't disagree with that in principle, but in a case like this, where the error is extremely easy to make, not warned about at compile time (so you have to exercise the failing code path to spot it), and won't even show up at runtime in the default build configuration (even once 64-bit SourceLocations are actually implemented, the current plan is for them to be off by default), I think it's worth at least trying to think of ways to help the caller get it right.

Perhaps an alternative API might be to replace SourceLocation::getLocWithOffset with an operator+, and add an operator- to go with it? Then you could write NewLoc = OldLoc + ThisInteger - ThatInteger and each integer would be implicitly widened if it was the wrong size.

But in this version of the patch, I haven't done that; I've just reverted the introduction of dyadic getLocWithOffset and added cumbersome casts or intermediate variables at the call sites where I'd previously used it.

clang/lib/CodeGen/CoverageMappingGen.cpp

235–238

That seems sensible from first principles, but I tried tugging on that thread and it turned out to have a lot of extra work behind it, because file offsets are absolutely all over the place (perhaps even more so that SourceLocations proper), and in particular, they're often used as indexes into in-memory buffers, which gets awkward if you want 64-bit SourceLocations and 64-bit builds of clang to be independent choices.

Perhaps you're right that expanding file offsets to 64-bit would also be a useful piece of work. But doing it as part of this work would double the size of the job, so I think it would make more sense to keep it separate.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

clang/lib/AST/SelectorLocationsKind.cpp

static SourceLocation getStandardSelLoc(unsigned Index,
                                         SourceLocation ArgLoc,
                                         SourceLocation EndLoc) {
   unsigned NumSelArgs = Sel.getNumArgs();
   if (NumSelArgs == 0) {
     assert(Index == 0);
     if (EndLoc.isInvalid())
       return SourceLocation();
     IdentifierInfo *II = Sel.getIdentifierInfoForSlot(0);
-    unsigned Len = II ? II->getLength() : 0;
+    SourceLocation::UIntTy Len = II ? II->getLength() : 0;
     return EndLoc.getLocWithOffset(-Len);
   }
 
   assert(Index < NumSelArgs);
   if (ArgLoc.isInvalid())
     return SourceLocation();
   IdentifierInfo *II = Sel.getIdentifierInfoForSlot(Index);
-  unsigned Len = /* selector id */ (II ? II->getLength() : 0) + /* ':' */ 1;
+  SourceLocation::UIntTy Len =
+      /* selector id */ (II ? II->getLength() : 0) + /* ':' */ 1;
   if (WithArgSpace)
     ++Len;
   return ArgLoc.getLocWithOffset(-Len);
 }
 
 namespace {
 
 template <typename T>
 SourceLocation getArgLoc(T* Arg);
 
 template <>

clang/lib/CodeGen/CoverageMappingGen.cpp

SourceLocation getPreciseTokenLocEnd(SourceLocation Loc) {
     // macro locations, which we just treat as expanded files.
     unsigned TokLen =
         Lexer::MeasureTokenLength(SM.getSpellingLoc(Loc), SM, LangOpts);
     return Loc.getLocWithOffset(TokLen);
   }
 
   /// Return the start location of an included file or expanded macro.
   SourceLocation getStartOfFileOrMacro(SourceLocation Loc) {
-    if (Loc.isMacroID())
-      return Loc.getLocWithOffset(-SM.getFileOffset(Loc));
+    if (Loc.isMacroID()) {
+      SourceLocation::UIntTy FileOffset = SM.getFileOffset(Loc);
+      return Loc.getLocWithOffset(-FileOffset);
+    }
     return SM.getLocForStartOfFile(SM.getFileID(Loc));
   }
 
   /// Return the end location of an included file or expanded macro.
   SourceLocation getEndOfFileOrMacro(SourceLocation Loc) {
     if (Loc.isMacroID())
       return Loc.getLocWithOffset(SM.getFileIDSize(SM.getFileID(Loc)) -
                                   SM.getFileOffset(Loc));

clang/lib/Format/FormatTokenLexer.cpp

FormatToken *FormatTokenLexer::getNextToken() {
   if (StateStack.top() == LexerState::TOKEN_STASHED) {
     StateStack.pop();
     return getStashedToken();
   }
 
   FormatTok = new (Allocator.Allocate()) FormatToken;
   readRawToken(*FormatTok);
   SourceLocation WhitespaceStart =
-      FormatTok->Tok.getLocation().getLocWithOffset(-TrailingWhitespace);
+      FormatTok->Tok.getLocation().getLocWithOffset(
+          -static_cast<SourceLocation::UIntTy>(TrailingWhitespace));
   FormatTok->IsFirst = IsFirstToken;
   IsFirstToken = false;
 
   // Consume and record whitespace until we find a significant token.
   unsigned WhitespaceLength = TrailingWhitespace;
   while (FormatTok->Tok.is(tok::unknown)) {
     StringRef Text = FormatTok->TokenText;
     auto EscapesNewline = [&](int pos) {

clang/lib/Lex/Lexer.cpp

static const char *findBeginningOfLine(StringRef Buffer, unsigned Offset) {
   }
   return LexStart;
 }
 
 static SourceLocation getBeginningOfFileToken(SourceLocation Loc,
                                               const SourceManager &SM,
                                               const LangOptions &LangOpts) {
   assert(Loc.isFileID());
-  std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(Loc);
-  if (LocInfo.first.isInvalid())
+  FileID LocFileID;
+  SourceLocation::UIntTy LocOffset;
+  std::tie(LocFileID, LocOffset) = SM.getDecomposedLoc(Loc);
+  if (LocFileID.isInvalid())
     return Loc;
 
   bool Invalid = false;
-  StringRef Buffer = SM.getBufferData(LocInfo.first, &Invalid);
+  StringRef Buffer = SM.getBufferData(LocFileID, &Invalid);
   if (Invalid)
     return Loc;
 
+  if (LocOffset > Buffer.size())
+    return Loc;
+
   // Back up from the current location until we hit the beginning of a line
   // (or the buffer). We'll relex from that point.
-  const char *StrData = Buffer.data() + LocInfo.second;
-  const char *LexStart = findBeginningOfLine(Buffer, LocInfo.second);
+  const char *StrData = Buffer.data() + LocOffset;
+  const char *LexStart = findBeginningOfLine(Buffer, LocOffset);
   if (!LexStart || LexStart == StrData)
     return Loc;
 
   // Create a lexer starting at the beginning of this token.
-  SourceLocation LexerStartLoc = Loc.getLocWithOffset(-LocInfo.second);
+  SourceLocation LexerStartLoc = Loc.getLocWithOffset(-LocOffset);
   Lexer TheLexer(LexerStartLoc, LangOpts, Buffer.data(), LexStart,
                  Buffer.end());
   TheLexer.SetCommentRetentionState(true);
 
   // Lex tokens until we find the token that contains the source location.
   Token TheTok;
   do {
     TheLexer.LexFromRawLexer(TheTok);
SourceLocation Lexer::GetBeginningOfToken(SourceLocation Loc,
   if (Loc.isFileID())
     return getBeginningOfFileToken(Loc, SM, LangOpts);
 
   if (!SM.isMacroArgExpansion(Loc))
     return Loc;
 
   SourceLocation FileLoc = SM.getSpellingLoc(Loc);
   SourceLocation BeginFileLoc = getBeginningOfFileToken(FileLoc, SM, LangOpts);
-  std::pair<FileID, unsigned> FileLocInfo = SM.getDecomposedLoc(FileLoc);
-  std::pair<FileID, unsigned> BeginFileLocInfo =
-      SM.getDecomposedLoc(BeginFileLoc);
-  assert(FileLocInfo.first == BeginFileLocInfo.first &&
-         FileLocInfo.second >= BeginFileLocInfo.second);
-  return Loc.getLocWithOffset(BeginFileLocInfo.second - FileLocInfo.second);
+  FileID FID, BeginFID;
+  SourceLocation::UIntTy FileOffset, BeginFileOffset;
+  std::tie(FID, FileOffset) = SM.getDecomposedLoc(FileLoc);
+  std::tie(BeginFID, BeginFileOffset) = SM.getDecomposedLoc(BeginFileLoc);
+  assert(FID == BeginFID && FileOffset >= BeginFileOffset);
+  return Loc.getLocWithOffset(BeginFileOffset - FileOffset);
 }
 
 namespace {
 
 enum PreambleDirectiveKind {
   PDK_Skipped,
   PDK_Unknown
 };

clang/lib/Parse/ParseStmtAsm.cpp

 SourceLocation
 ClangAsmParserCallback::translateLocation(const llvm::SourceMgr &LSM,
                                           llvm::SMLoc SMLoc) {
   // Compute an offset into the inline asm buffer.
   // FIXME: This isn't right if .macro is involved (but hopefully, no
   // real-world code does that).
   const llvm::MemoryBuffer *LBuf =
       LSM.getMemoryBuffer(LSM.FindBufferContainingLoc(SMLoc));
   unsigned Offset = SMLoc.getPointer() - LBuf->getBufferStart();
 
   // Figure out which token that offset points into.
   const unsigned *TokOffsetPtr = llvm::lower_bound(AsmTokOffsets, Offset);
   unsigned TokIndex = TokOffsetPtr - AsmTokOffsets.begin();
   unsigned TokOffset = *TokOffsetPtr;
 
   // If we come up with an answer which seems sane, use it; otherwise,
   // just point at the __asm keyword.
   // FIXME: Assert the answer is sane once we handle .macro correctly.
   SourceLocation Loc = AsmLoc;
   if (TokIndex < AsmToks.size()) {
     const Token &Tok = AsmToks[TokIndex];
     Loc = Tok.getLocation();
-    Loc = Loc.getLocWithOffset(Offset - TokOffset);
+    Loc = Loc.getLocWithOffset(static_cast<SourceLocation::UIntTy>(Offset) -
+                               TokOffset);
   }
   return Loc;
 }
 
 void ClangAsmParserCallback::handleDiagnostic(const llvm::SMDiagnostic &D) {
   const llvm::SourceMgr &LSM = *D.getSourceMgr();
   SourceLocation Loc = translateLocation(LSM, D.getLoc());
   TheParser.Diag(Loc, diag::err_inline_ms_asm_parsing) << D.getMessage();

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

void HTMLDiagnostics::HighlightRange(Rewriter& R, FileID BugFileID,
   if (EndLineNo < StartLineNo)
     return;
 
   if (SM.getFileID(InstantiationStart) != BugFileID ||
       SM.getFileID(InstantiationEnd) != BugFileID)
     return;
 
   // Compute the column number of the end.
   unsigned EndColNo = SM.getExpansionColumnNumber(InstantiationEnd);
   unsigned OldEndColNo = EndColNo;
 
   if (EndColNo) {
     // Add in the length of the token, so that we cover multi-char tokens.
     EndColNo += Lexer::MeasureTokenLength(Range.getEnd(), SM, LangOpts)-1;
   }
 
   // Highlight the range.  Make the span tag the outermost tag for the
   // selected range.
 
-  SourceLocation E =
-    InstantiationEnd.getLocWithOffset(EndColNo - OldEndColNo);
+  SourceLocation E = InstantiationEnd.getLocWithOffset(
+      static_cast<SourceLocation::UIntTy>(EndColNo) - OldEndColNo);
 
   html::HighlightRange(R, InstantiationStart, E, HighlightStart, HighlightEnd);
 }
 
 StringRef HTMLDiagnostics::generateKeyboardNavigationJavascript() {
   return R"<<<(
 <script type='text/javascript'>
 var digitMatcher = new RegExp("[0-9]+");