-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[Safe Buffers] Serialize unsafe_buffer_usage pragmas #92031
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ac5aeb5
23050bc
64b1f95
72805ea
320725c
2360af2
7c86acb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -58,6 +58,7 @@ | |||
#include "llvm/ADT/SmallString.h" | ||||
#include "llvm/ADT/SmallVector.h" | ||||
#include "llvm/ADT/StringRef.h" | ||||
#include "llvm/ADT/iterator_range.h" | ||||
#include "llvm/Support/Capacity.h" | ||||
#include "llvm/Support/ErrorHandling.h" | ||||
#include "llvm/Support/MemoryBuffer.h" | ||||
|
@@ -1483,26 +1484,56 @@ void Preprocessor::emitFinalMacroWarning(const Token &Identifier, | |||
} | ||||
|
||||
bool Preprocessor::isSafeBufferOptOut(const SourceManager &SourceMgr, | ||||
const SourceLocation &Loc) const { | ||||
// Try to find a region in `SafeBufferOptOutMap` where `Loc` is in: | ||||
auto FirstRegionEndingAfterLoc = llvm::partition_point( | ||||
SafeBufferOptOutMap, | ||||
[&SourceMgr, | ||||
&Loc](const std::pair<SourceLocation, SourceLocation> &Region) { | ||||
return SourceMgr.isBeforeInTranslationUnit(Region.second, Loc); | ||||
}); | ||||
const SourceLocation &Loc) const { | ||||
// The lambda that tests if a `Loc` is in an opt-out region given one opt-out | ||||
// region map: | ||||
auto TestInMap = [&SourceMgr](const SafeBufferOptOutRegionsTy &Map, | ||||
const SourceLocation &Loc) -> bool { | ||||
// Try to find a region in `SafeBufferOptOutMap` where `Loc` is in: | ||||
auto FirstRegionEndingAfterLoc = llvm::partition_point( | ||||
Map, [&SourceMgr, | ||||
&Loc](const std::pair<SourceLocation, SourceLocation> &Region) { | ||||
return SourceMgr.isBeforeInTranslationUnit(Region.second, Loc); | ||||
}); | ||||
|
||||
if (FirstRegionEndingAfterLoc != Map.end()) { | ||||
// To test if the start location of the found region precedes `Loc`: | ||||
return SourceMgr.isBeforeInTranslationUnit( | ||||
FirstRegionEndingAfterLoc->first, Loc); | ||||
} | ||||
// If we do not find a region whose end location passes `Loc`, we want to | ||||
// check if the current region is still open: | ||||
if (!Map.empty() && Map.back().first == Map.back().second) | ||||
return SourceMgr.isBeforeInTranslationUnit(Map.back().first, Loc); | ||||
return false; | ||||
}; | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok @ziqingluo-90 has just walked me through this offline and I think I understand what's happening now. What was unobvious to me: shouldn't we
Let's add a comment explaining this! |
||||
if (FirstRegionEndingAfterLoc != SafeBufferOptOutMap.end()) { | ||||
// To test if the start location of the found region precedes `Loc`: | ||||
return SourceMgr.isBeforeInTranslationUnit(FirstRegionEndingAfterLoc->first, | ||||
Loc); | ||||
} | ||||
// If we do not find a region whose end location passes `Loc`, we want to | ||||
// check if the current region is still open: | ||||
if (!SafeBufferOptOutMap.empty() && | ||||
SafeBufferOptOutMap.back().first == SafeBufferOptOutMap.back().second) | ||||
return SourceMgr.isBeforeInTranslationUnit(SafeBufferOptOutMap.back().first, | ||||
Loc); | ||||
// What the following does: | ||||
// | ||||
// If `Loc` belongs to the local TU, we just look up `SafeBufferOptOutMap`. | ||||
// Otherwise, `Loc` is from a loaded AST. We look up the | ||||
// `LoadedSafeBufferOptOutMap` first to get the opt-out region map of the | ||||
// loaded AST where `Loc` is at. Then we find if `Loc` is in an opt-out | ||||
// region w.r.t. the region map. If the region map is absent, it means there | ||||
// is no opt-out pragma in that loaded AST. | ||||
// | ||||
// Opt-out pragmas in the local TU or a loaded AST is not visible to another | ||||
// one of them. That means if you put the pragmas around a `#include | ||||
// "module.h"`, where module.h is a module, it is not actually suppressing | ||||
// warnings in module.h. This is fine because warnings in module.h will be | ||||
// reported when module.h is compiled in isolation and nothing in module.h | ||||
// will be analyzed ever again. So you will not see warnings from the file | ||||
// that imports module.h anyway. And you can't even do the same thing for PCHs | ||||
// because they can only be included from the command line. | ||||
|
||||
if (SourceMgr.isLocalSourceLocation(Loc)) | ||||
return TestInMap(SafeBufferOptOutMap, Loc); | ||||
|
||||
const SafeBufferOptOutRegionsTy *LoadedRegions = | ||||
LoadedSafeBufferOptOutMap.lookupLoadedOptOutMap(Loc, SourceMgr); | ||||
|
||||
if (LoadedRegions) | ||||
return TestInMap(*LoadedRegions, Loc); | ||||
return false; | ||||
} | ||||
|
||||
|
@@ -1551,6 +1582,47 @@ bool Preprocessor::isPPInSafeBufferOptOutRegion(SourceLocation &StartLoc) { | |||
return InSafeBufferOptOutRegion; | ||||
} | ||||
|
||||
SmallVector<SourceLocation, 64> | ||||
Preprocessor::serializeSafeBufferOptOutMap() const { | ||||
assert(!InSafeBufferOptOutRegion && | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We require the pragma to be closed before the end of TU right? And that's a hard error, not a warning? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a warning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah in this case we need to support the situation when the warning was ignored. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I'm sorry, they are not warnings, they are errors: llvm-project/clang/lib/Lex/Pragma.cpp Line 1265 in c796900
|
||||
"Attempt to serialize safe buffer opt-out regions before file being " | ||||
"completely preprocessed"); | ||||
|
||||
SmallVector<SourceLocation, 64> SrcSeq; | ||||
|
||||
for (const auto &[begin, end] : SafeBufferOptOutMap) { | ||||
SrcSeq.push_back(begin); | ||||
SrcSeq.push_back(end); | ||||
} | ||||
// Only `SafeBufferOptOutMap` gets serialized. No need to serialize | ||||
// `LoadedSafeBufferOptOutMap` because if this TU loads a pch/module, every | ||||
// pch/module in the pch-chain/module-DAG will be loaded one by one in order. | ||||
// It means that for each loading pch/module m, it just needs to load m's own | ||||
// `SafeBufferOptOutMap`. | ||||
return SrcSeq; | ||||
} | ||||
|
||||
bool Preprocessor::setDeserializedSafeBufferOptOutMap( | ||||
const SmallVectorImpl<SourceLocation> &SourceLocations) { | ||||
if (SourceLocations.size() == 0) | ||||
return false; | ||||
|
||||
assert(SourceLocations.size() % 2 == 0 && | ||||
"ill-formed SourceLocation sequence"); | ||||
|
||||
auto It = SourceLocations.begin(); | ||||
SafeBufferOptOutRegionsTy &Regions = | ||||
LoadedSafeBufferOptOutMap.findAndConsLoadedOptOutMap(*It, SourceMgr); | ||||
|
||||
do { | ||||
SourceLocation Begin = *It++; | ||||
SourceLocation End = *It++; | ||||
|
||||
Regions.emplace_back(Begin, End); | ||||
} while (It != SourceLocations.end()); | ||||
return true; | ||||
} | ||||
|
||||
ModuleLoader::~ModuleLoader() = default; | ||||
|
||||
CommentHandler::~CommentHandler() = default; | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite magical to me because I don't understand how FileIDs are assigned and in what order they show up in the
LoadedSLocEntryAllocBegin
list. But that's probably ok, this looks like completely normalSourceManager
code to me.