https://github.com/ilya-biryukov closed
https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/ilya-biryukov updated
https://github.com/llvm/llvm-project/pull/79875
>From db8b091568c924a9550052ea643f718ac11d3e73 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov
Date: Mon, 29 Jan 2024 18:55:53 +0100
Subject: [PATCH 1/4] [Serialization] Check for stack exhaustion when
ilya-biryukov wrote:
I have tried `ulimit` and found that Clang relies on a very intricate balance
between the stack size, the desired stack size (the cutoff at which we create a
new thread with a larger stack) and the points in code where we call
`runWithSufficientStackSpace`.
If the stack
https://github.com/ilya-biryukov updated
https://github.com/llvm/llvm-project/pull/79875
>From 6e474e393e63fd1cb5f3b0bea3c971b96591c57f Mon Sep 17 00:00:00 2001
From: Ilya Biryukov
Date: Mon, 29 Jan 2024 18:55:53 +0100
Subject: [PATCH 1/4] [Serialization] Check for stack exhaustion when
@@ -9403,6 +9404,20 @@ DiagnosticBuilder ASTReader::Diag(SourceLocation Loc,
unsigned DiagID) const {
return Diags.Report(Loc, DiagID);
}
+void ASTReader::warnStackExhausted(SourceLocation Loc) {
+ // When Sema is available, avoid duplicate errors. This should be rare.
https://github.com/ChuanqiXu9 edited
https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/ChuanqiXu9 approved this pull request.
If we don't insist on testing this in this commit, them LGTM.
https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
@@ -9403,6 +9404,20 @@ DiagnosticBuilder ASTReader::Diag(SourceLocation Loc,
unsigned DiagID) const {
return Diags.Report(Loc, DiagID);
}
+void ASTReader::warnStackExhausted(SourceLocation Loc) {
+ // When Sema is available, avoid duplicate errors. This should be rare.
ilya-biryukov wrote:
> there's a couple of tests that use `ulimit`
> (`clang/test/SemaCXX/PR51712-large-array-constexpr-check-oom.cpp` and
> `clang/test/PCH/leakfiles.test`) - so that technique could be used to test
> this in a way that's fast enough to check in?
oh, I didn't realize I can
dwblaikie wrote:
(a bunch of compiler-rt tests also use ulimit, but doesn't look like any llvm
core tests do... )
https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dwblaikie wrote:
there's a couple of tests that use `ulimit`
(`clang/test/SemaCXX/PR51712-large-array-constexpr-check-oom.cpp` and
`clang/test/PCH/leakfiles.test`) - so that technique could be used to test this
in a way that's fast enough to check in?
ilya-biryukov wrote:
> Yeah, I was hoping to have it in the text of the discussion here without
> having to do it myself since you've already got the repro locally,
> presumably... so we can all see/discuss it. But perhaps it's not sufficiently
> helpful/constructive to worry about - not
@@ -4099,7 +4099,9 @@ Decl *ASTReader::ReadDeclRecord(DeclID ID) {
// calls to Decl::getASTContext() by Decl's methods will find the
// TranslationUnitDecl without crashing.
D->setDeclContext(Context.getTranslationUnitDecl());
- Reader.Visit(D);
+
+ // Reading some
https://github.com/ilya-biryukov updated
https://github.com/llvm/llvm-project/pull/79875
>From 6e474e393e63fd1cb5f3b0bea3c971b96591c57f Mon Sep 17 00:00:00 2001
From: Ilya Biryukov
Date: Mon, 29 Jan 2024 18:55:53 +0100
Subject: [PATCH 1/3] [Serialization] Check for stack exhaustion when
cor3ntin wrote:
@ilya-biryukov ping!
https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
AaronBallman wrote:
> I think we should apply @ChuanqiXu9 suggestion and merge that without tests.
> At worse it's harmless, at best it solves an actual issue for users. We have
> precedent for not being able to test resource exhaustion fixes.
I would not be opposed to landing with a test,
ilya-biryukov wrote:
Sorry for loosing track of this change. I will go through the comments and try
to land this some time next week.
https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
cor3ntin wrote:
I think we should apply @ChuanqiXu9 suggestion and merge that without tests. At
worse it's harmless, at best it solves an actual issue for users. We have
precedent for not being able to test resource exhaustion fixes.
https://github.com/llvm/llvm-project/pull/79875
dwblaikie wrote:
> > Could you show the stack (omitting/annotating the repeated part) that leads
> > to failure? and/or the AST shape that leads to failure?
>
> See the test I added. All you need is ~10k overloads of a method in a class
> and a `using Base::func` in the derived class. The AST
ChuanqiXu9 wrote:
> with stack exhaustion warning, the compiler can continue (albeit being slow
> or unstable). The depth limit here will be a hard restriction and so there
> will be no workaround if the code reaches it.
It is a surprise to me that this is only a warning instead of a hard
ChuanqiXu9 wrote:
> I am not saying it's a bad idea to add this for testing purposes, but given
> how fragile this approach is, I think we should not provide configuration
> knobs to users there. At least everyone sees crashes at roughly the same
> points right now (although variation is
@@ -4099,7 +4099,9 @@ Decl *ASTReader::ReadDeclRecord(DeclID ID) {
// calls to Decl::getASTContext() by Decl's methods will find the
// TranslationUnitDecl without crashing.
D->setDeclContext(Context.getTranslationUnitDecl());
- Reader.Visit(D);
+
+ // Reading some
ilya-biryukov wrote:
> To make this testable, maybe we can refactor
> `clang::runWithSufficientStackSpace` a little bit to make `DesiredStackSize`
> and `isStackNearlyExhausted::SufficientStack` configurable.
Maybe... As long as we only use this in tests.
However, for this particular case, we
@@ -4099,7 +4099,9 @@ Decl *ASTReader::ReadDeclRecord(DeclID ID) {
// calls to Decl::getASTContext() by Decl's methods will find the
// TranslationUnitDecl without crashing.
D->setDeclContext(Context.getTranslationUnitDecl());
- Reader.Visit(D);
+
+ // Reading some
ilya-biryukov wrote:
> Could you show the stack (omitting/annotating the repeated part) that leads
> to failure? and/or the AST shape that leads to failure?
See the test I added. All you need is ~10k overloads of a method in a class and
a `using Base::func` in the derived class.\
The AST
@@ -0,0 +1,43 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/usings.cppm -o
%t/usings.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=usings=%t/usings.pcm %t/use.cpp
-verify -fsyntax-only
ChuanqiXu9 wrote:
Another idea is to limit (or check) the threshold for
`ASTReader::NumCurrentElementsDeserializing`. Then we still can print the stack
trace by some utils in LLVM also we can get better performance than this.
https://github.com/llvm/llvm-project/pull/79875
@@ -0,0 +1,43 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/usings.cppm -o
%t/usings.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=usings=%t/usings.pcm %t/use.cpp
-verify -fsyntax-only
https://github.com/ChuanqiXu9 commented:
To make this testable, maybe we can refactor
`clang::runWithSufficientStackSpace` a little bit to make `DesiredStackSize`
and `isStackNearlyExhausted::SufficientStack` configurable.
https://github.com/llvm/llvm-project/pull/79875
@@ -4099,7 +4099,9 @@ Decl *ASTReader::ReadDeclRecord(DeclID ID) {
// calls to Decl::getASTContext() by Decl's methods will find the
// TranslationUnitDecl without crashing.
D->setDeclContext(Context.getTranslationUnitDecl());
- Reader.Visit(D);
+
+ // Reading some
https://github.com/ChuanqiXu9 edited
https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dwblaikie wrote:
Could you show the stack (omitting/annotating the repeated part) that leads to
failure? and/or the AST shape that leads to failure?
https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
https://github.com/cor3ntin approved this pull request.
I think it makes sense to make this change without tests
https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
@@ -0,0 +1,43 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/usings.cppm -o
%t/usings.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=usings=%t/usings.pcm %t/use.cpp
-verify -fsyntax-only
https://github.com/ilya-biryukov commented:
I think the alternative we could try to chase is turning the chain of
using-shadow-decls into an array somehow, so we can process it in a loop
instead of recursion.
I am not sure if there is an an approach that does **not** involve structural
https://github.com/ilya-biryukov edited
https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
llvmbot wrote:
@llvm/pr-subscribers-clang-modules
@llvm/pr-subscribers-clang
Author: Ilya Biryukov (ilya-biryukov)
Changes
Particular example that lead to this is a very long chain of `UsingShadowDecl`s
that we hit in our codebase in generated code.
To avoid that, check for stack
https://github.com/ilya-biryukov created
https://github.com/llvm/llvm-project/pull/79875
Particular example that lead to this is a very long chain of `UsingShadowDecl`s
that we hit in our codebase in generated code.
To avoid that, check for stack exhaustion when deserializing the declaration.
38 matches
Mail list logo
