[PATCH] D55250: [clangd] Enhance macro hover to see full definition

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/XRefs.cpp:578
+    bool Invalid;
+    StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid);
+    if (!Invalid) {
----------------
ilya-biryukov wrote:
> ilya-biryukov wrote:
> > malaperle wrote:
> > > ilya-biryukov wrote:
> > > > Unfortunately we can't get the buffer here, because for the preamble macros the files might change on disk after we build the preamble and we can end up reading a different version of the file. Which can in turn lead to crashes as the offsets obtained from the source locations can point outside the buffer for the corresponding file.
> > > > 
> > > > This is really annoying and generally the solution is to try find a different way to obtain the same information, e.g. by looking at what `MacroInfo` has available.
> > > > However, I don't know of a good workaround for this. Happy to look into ways of providing something close to a macro definition from `MacroInfo` or other sources, can scout for this tomorrow.
> > > I tried to do something like MacroInfo::dump. One problem is that we don't always have enough information about literals. For example:
> > > 
> > > ```
> > > #define MACRO 0
> > > int main() { return MACRO; }
> > > ```
> > > Becomes:
> > > ```
> > > #define MACRO numeric_constant
> > > ```
> > > 
> > > I poked around the Token code and I didn't find a way to retrieve the literal without going through the buffer (Preprocessor::getSpelling for example).
> > > 
> > > What you mention is only a problem when you use the option of storing preambles on disk, right? So something would need to modify the preamble files on disk, which are stored in some temp folder. It doesn't sound like that could happen frequently. There is also bounds checking in the code above so it shouldn't crash, right? Even if the bounds are incorrect, it will be no different than the Range we return to the client for document/definition, i.e. the client can use the range on a newly modified header and get it wrong. I also tested renaming the pch file and then editing the source file and it results in an invalid AST. So either way modifying the pch would be bad news not for just macro hover.
> > The problem shows up when the header files are modified, not the preamble files (otherwise we'll be fine, in clangd we do assume the .pch files we produce are not externally modified).
> > Which is pretty frequent, and we've seen real crashes coming from fetching documentation from the preambles.
> > 
> > I'll try to investigate how Preprocessor accesses the tokens of macro definitions from preabmle. There are two possible outcomes I expect:
> > 1. we find a way to access the spelling of the tokens in the same way PP does and it's safe,
> > 2. we realize preprocessor might crash with the preabmle because it accesses the spelling of the tokens.
> > 
> > I hope to find (1), but (2) is also very realistic, unfortunately :-(
> I think the preprocessor does the same thing, therefore the compiler can potentially access the header files to get spelling of the tokens coming from headers.
> This means that clangd can potentially crash if the headers are changed concurrently with the operations that use preabmles (code completion, being the most common one).
> 
> In that case, I agree that it's fine to use the buffers for headers here and we should fix this separately, probably by snapshotting the contents of accessed headers.
And I'm happy to take a look at reproducing the PP crash that I'm talking about here and fixing this.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55250