This is an archive of the discontinued LLVM Phabricator instance.

[clangd] WIP: fix several bugs relating to include insertion
AcceptedPublic

Authored by sammccall on Apr 13 2020, 10:25 AM.

Details

Summary

This should probably be untangled into 3 separate patches, but
looking for feedback first on whether we want to do this at all.

a) store include-insertion information for main-file symbols that are

not eligible for code completion indexing.

b) consider an #ifndef guard that's dangling at the end of the preamble

to be a well-formed header guard (otherwise we never will)

c) load HeaderFileInfo for the preamble main-file even if the file-size

mismatches

Diff Detail

Event Timeline

sammccall created this revision.Apr 13 2020, 10:25 AM
kadircet added inline comments.Apr 13 2020, 12:30 PM
clang/lib/Lex/Lexer.cpp
2749

I think we should put this behind a PPOpt to make sure we don't regress the rest of the world, as I fear this part of the code might not be well tested.

clang/lib/Serialization/ASTReader.cpp
6155

do we have other (apart from isFileMultiIncludeGuarded) things that depend on HFI for main file being correctly deserialized?

If it is only the include guard, maybe we can store that info in control block and later on restore it when reading HFI for main file in HeaderSearch::getExistingFileInfo?

nridge added a subscriber: nridge.Jul 6 2022, 6:52 PM

@sammccall should we get this patch landed? The issue of #ifndef-guarded header files that include each other recursively is still tripping up some users.

Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJul 6 2022, 6:52 PM

We stumbled upon this issue too. What is needed to pull in this patch?

Let's try adding some reviewers

Also adding links to the open issues this fixes:

Note the second issue is not specific to include insertion; it's a false positive diagnostic, which I think makes it a more severe bug than include insertion misbehaving. That issue likely has some duplicates lying around (at least I remember it coming up multiple times) though I can't find them at the moment.

sammccall updated this revision to Diff 540981.Jul 17 2023, 5:57 AM

rebase and narrow scope

sammccall updated this revision to Diff 540985.Jul 17 2023, 6:01 AM
sammccall marked an inline comment as done.

oops

OK, time to revive this patch, sorry for letting it die.

Meanwhile, the lexer change has landed separately, so that's gone.

I've removed the gratuitous clangd indexing changes in order to focus this on the serialization hacks.

clang/lib/Lex/Lexer.cpp
2749

(this is obsolete as this code landed separately)

clang/lib/Serialization/ASTReader.cpp
6155

The real answer here is "I have no idea". I definitely don't have bug reports saying other things are broken.
HFI looks like it affects module semantics, so seems like it could be relevant when the main file is part of a module. But today we don't have modules support...

Really, I'd just rather make the existing mechanism work than add a second one.

kadircet accepted this revision.Jul 19 2023, 4:41 AM

thanks, i think this LG.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
1296–1297

headerSymbols still uses Filename not HeaderFilename of the TU.

1298–1301

i think we don't want to just check for names of the symbols, but also want to make sure they got proper include headers assigned?

This revision is now accepted and ready to land.Jul 19 2023, 4:41 AM