-
Notifications
You must be signed in to change notification settings - Fork 12.2k
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
clangd crashes when including some standard library headers in the global module fragment #80570
Comments
@llvm/issue-subscribers-clangd Author: None (marwing)
The following code currently reliably crashes clangd.
I am using a nightly llvm build (d9850fe) with current neovim HEAD and the built-in lsp client.
I bisected this crash to a0b6747. Considering this is only using libc++ headers from the same commit, I don't include preprocessed sources. If you need them anyway, I can provide them. Reproducer: // test.cpp
module;
// any one off these is enough to crash clangd
// #include <iostream>
#include <string>
// #include <string_view>
// #include <cmath>
// #include <system_error>
// #include <new>
// #include <bit>
// probably many more
// only ok with libc++, not the system provided libstdc++ 13.2.1
#include <iosfwd>
// these are ok
#include <cstddef>
#include <cerrno>
export module test;
<details>
</details> <details>
</details> |
Please note that module support of clangd is incomplete and tracked at clangd/clangd#1293. |
I know, thanks. I'm just toying around with modules to improve my understanding of them and a crash is bad regardless. |
This fix partially reverts llvm@a0b6747. The serialization part is restored to the state prior to the mentioned commit as it causing issue with reading back Decl. ODR checks logic is kept in place. Close llvm#80570.
The reason may be that the clangd serialize the headers into preamble files with ODRHash and when we read them we skipped the ODRHash. |
Close llvm#80570. In llvm@a0b6747, we skipped ODR checks for decls in GMF. Then it should be natural to skip storing the ODR values in BMI. Generally it should be fine as long as the writer and the reader keep consistent. However, the use of preamble in clangd shows the tricky part. For, ``` // test.cpp module; // any one off these is enough to crash clangd // #include <iostream> // #include <string_view> // #include <cmath> // #include <system_error> // #include <new> // #include <bit> // probably many more // only ok with libc++, not the system provided libstdc++ 13.2.1 // these are ok export module test; ``` clangd will store the headers as preamble to speedup the parsing and the preamble reuses the serialization techniques. (Generally we'd call the preamble as PCH. However it is not true strictly. I've tested the PCH wouldn't be problematic.) However, the tricky part is that the preamble is not modules. It literally serialiaze and deserialize things. So before clangd parsing the above test module, clangd will serialize the headers into the preamble. Note that there is no concept like GMF now. So the ODR bits are stored. However, when clangd parse the file actually, the decls from preamble are thought as in GMF literally, then hte ODR bits are skipped. Then mismatch happens. To solve the problem, this patch adds another bit for decls to record whether or not the ODR bits are skipped.
Close #80570. In a0b6747, we skipped ODR checks for decls in GMF. Then it should be natural to skip storing the ODR values in BMI. Generally it should be fine as long as the writer and the reader keep consistent. However, the use of preamble in clangd shows the tricky part. For, ``` // test.cpp module; // any one off these is enough to crash clangd // #include <iostream> // #include <string_view> // #include <cmath> // #include <system_error> // #include <new> // #include <bit> // probably many more // only ok with libc++, not the system provided libstdc++ 13.2.1 // these are ok export module test; ``` clangd will store the headers as preamble to speedup the parsing and the preamble reuses the serialization techniques. (Generally we'd call the preamble as PCH. However it is not true strictly. I've tested the PCH wouldn't be problematic.) However, the tricky part is that the preamble is not modules. It literally serialiaze and deserialize things. So before clangd parsing the above test module, clangd will serialize the headers into the preamble. Note that there is no concept like GMF now. So the ODR bits are stored. However, when clangd parse the file actually, the decls from preamble are thought as in GMF literally, then hte ODR bits are skipped. Then mismatch happens. To solve the problem, this patch adds another bit for decls to record whether or not the ODR bits are skipped.
@llvm/issue-subscribers-clang-frontend Author: None (marwing)
The following code currently reliably crashes clangd.
I am using a nightly llvm build (d9850fe) with current neovim HEAD and the built-in lsp client.
I bisected this crash to a0b6747. Considering this is only using libc++ headers from the same commit, I don't include preprocessed sources. If you need them anyway, I can provide them. Reproducer: // test.cpp
module;
// any one off these is enough to crash clangd
// #include <iostream>
#include <string>
// #include <string_view>
// #include <cmath>
// #include <system_error>
// #include <new>
// #include <bit>
// probably many more
// only ok with libc++, not the system provided libstdc++ 13.2.1
#include <iosfwd>
// these are ok
#include <cstddef>
#include <cerrno>
export module test;
<details>
</details> <details>
</details> |
/cherry-pick 49775b1 |
Although we announced that the support for C++20 modules in clangd is broken, there are already a lot of people tried to use clangd with modules in some level. So it should be helpful to backport this to clang18 as well to improve the user experience as much as possible. |
Close llvm#80570. In llvm@a0b6747, we skipped ODR checks for decls in GMF. Then it should be natural to skip storing the ODR values in BMI. Generally it should be fine as long as the writer and the reader keep consistent. However, the use of preamble in clangd shows the tricky part. For, ``` // test.cpp module; // any one off these is enough to crash clangd // #include <iostream> // #include <string_view> // #include <cmath> // #include <system_error> // #include <new> // #include <bit> // probably many more // only ok with libc++, not the system provided libstdc++ 13.2.1 // these are ok export module test; ``` clangd will store the headers as preamble to speedup the parsing and the preamble reuses the serialization techniques. (Generally we'd call the preamble as PCH. However it is not true strictly. I've tested the PCH wouldn't be problematic.) However, the tricky part is that the preamble is not modules. It literally serialiaze and deserialize things. So before clangd parsing the above test module, clangd will serialize the headers into the preamble. Note that there is no concept like GMF now. So the ODR bits are stored. However, when clangd parse the file actually, the decls from preamble are thought as in GMF literally, then hte ODR bits are skipped. Then mismatch happens. To solve the problem, this patch adds another bit for decls to record whether or not the ODR bits are skipped. (cherry picked from commit 49775b1)
/pull-request #82309 |
Close llvm#80570. In llvm@a0b6747, we skipped ODR checks for decls in GMF. Then it should be natural to skip storing the ODR values in BMI. Generally it should be fine as long as the writer and the reader keep consistent. However, the use of preamble in clangd shows the tricky part. For, ``` // test.cpp module; // any one off these is enough to crash clangd // #include <iostream> // #include <string_view> // #include <cmath> // #include <system_error> // #include <new> // #include <bit> // probably many more // only ok with libc++, not the system provided libstdc++ 13.2.1 // these are ok export module test; ``` clangd will store the headers as preamble to speedup the parsing and the preamble reuses the serialization techniques. (Generally we'd call the preamble as PCH. However it is not true strictly. I've tested the PCH wouldn't be problematic.) However, the tricky part is that the preamble is not modules. It literally serialiaze and deserialize things. So before clangd parsing the above test module, clangd will serialize the headers into the preamble. Note that there is no concept like GMF now. So the ODR bits are stored. However, when clangd parse the file actually, the decls from preamble are thought as in GMF literally, then hte ODR bits are skipped. Then mismatch happens. To solve the problem, this patch adds another bit for decls to record whether or not the ODR bits are skipped. (cherry picked from commit 49775b1)
The following code currently reliably crashes clangd.
I am using a nightly llvm build (d9850fe) with current neovim HEAD and the built-in lsp client.
I bisected this crash to a0b6747.
Considering this is only using libc++ headers from the same commit, I don't include preprocessed sources. If you need them anyway, I can provide them.
Reproducer:
clangd log
Just in case another log is helpful,
clangd --check
fails with the same assertion.The text was updated successfully, but these errors were encountered: