Page MenuHomePhabricator

[clangd] IncludeCleaner: Do not process locations in built-in files
ClosedPublic

Authored by kbobyrev on Oct 27 2021, 2:36 AM.

Details

Summary

Doing otherwise leads to crashing. Way to reproduce: open "gmock/gmock.h" in
the LLVM source tree.

Diff Detail

Event Timeline

kbobyrev created this revision.Oct 27 2021, 2:36 AM
kbobyrev requested review of this revision.Oct 27 2021, 2:36 AM
kbobyrev updated this revision to Diff 382608.Oct 27 2021, 4:45 AM

Also handle the Command-Line defines.

kbobyrev edited reviewers, added: kadircet; removed: sammccall.Oct 27 2021, 4:45 AM
kbobyrev added a subscriber: sammccall.
kadircet accepted this revision.Oct 27 2021, 10:24 AM

thanks, lgtm!

clang-tools-extra/clangd/IncludeCleaner.cpp
133

I couldn't figure out why macros defined via CLI are ending up in <built-in> file, despite having the right line directive in https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/InitPreprocessor.cpp#L1257. Looks like a bug in SourceManager. But still let's check for that too, just to be on the safe side (i.e. isWrittenInCommandLineFile)

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
245

you can change this to int y = CLI + __llvm__ to explicitly check for both CLI and built-in macros.

This revision is now accepted and ready to land.Oct 27 2021, 10:24 AM
kbobyrev updated this revision to Diff 382713.Oct 27 2021, 10:31 AM
kbobyrev marked 2 inline comments as done.

Resolve review comments.

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
245

Ahh, that's a nice built-in! Thank you!

This revision was landed with ongoing or failed builds.Oct 27 2021, 10:31 AM
This revision was automatically updated to reflect the committed changes.

Sorry to be late to the party, just some efficiency concerns

clang-tools-extra/clangd/IncludeCleaner.cpp
133

This part of the function is a hot path: we haven't deduplicated FileIDs yet. And isWrittenInBulitinFile/CommandLineFile are not cheap (seriously, go look at the implementation of getPresumedLoc. I'm not sure *why* we need to handle the 'presumed' cases there, either).

I think the simple/good thing is to allow this to hit Files.insert(FID) and then filter those out later instead.

This could be in a simple walk over the DenseSet at the end, but translateToHeaderIDs is actually looking at the FileEntries for each header anyway. My guess is we're crashing in this code:

const FileEntry *FE = SM.getFileEntryForID(FID);
assert(FE); // this assert passes, we get a fake FE for "<built in>", "<command line>" or "<scratch>"
// Option 1: we could bail out here with a simple check on FE->getName().
const auto File = Includes.getID(FE);
assert(File); // this assert fails. Option 2: turn this assert into an if instead.
148

FWIW, the same seems to apply here (though at least we've deduplicated file IDs).

I don't think we need to do the expensive check to avoid adding scratch buffers to the list, when we can just filter them out at the end with a cheaper check.
(In any case I don't think it makes much sense to check for scratch *before* calling add(), but check for builtin/cli *inside* add()).

I'll try some things out and send a patch...

Sorry to be late to the party, just some efficiency concerns

No problem, thank you for the comments, I'll send a follow-up!

clang-tools-extra/clangd/IncludeCleaner.cpp
133

Hmm, good idea, I'll resolve this in a follow-up!

P.S. Yes, we're crashing on assert(FE) in here.

148

Yeah, I was trying to figure out if this would apply to the macro expansions/spellings but couldn't figure out a reasonable example, so I left it out. I agree that it should probably be handled, too, I just wasn't sure when this could happen.

If we do post-filtering, I guess we can handle all of the locations at once, so probably safe to even remove these checks.

I'll try some things out and send a patch...

Oh, okay, I won't do mine then to avoid crashing into each other.