Details
-
Bug
-
Status: Closed (View Workflow)
-
Major
-
Resolution: Fixed
-
10.5
Description
After update to latest GCC 10, I see the aforementioned test failing. I use -flto that enables cross module inlining and I investigated that stack overflow detection does not work in `check_stack_overrun` function in sql_parse.cc. It's very likely caused by optimized out the allocation of buf argument of the function, which is a stack variable in a caller frame.
I'm suggesting the following patch:
diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
|
index a8e66d2a..9cbf23d7 100644 |
--- a/sql/sql_parse.cc
|
+++ b/sql/sql_parse.cc
|
@@ -7229,10 +7229,14 @@ long max_stack_used; |
corresponding exec. (Thus we only have to check in fix_fields.)
|
- Passing to check_stack_overrun() prevents the compiler from removing it.
|
*/
|
-bool check_stack_overrun(THD *thd, long margin, |
- uchar *buf __attribute__((unused)))
|
+bool check_stack_overrun(THD *thd, long margin, uchar *buf) |
{
|
long stack_used; |
+ /* |
+ Take address of the buf argument in order to prevent
|
+ the stack allocation made this function caller.
|
+ */
|
+ static volatile uchar *buf_ptr = buf; |
DBUG_ASSERT(thd == current_thd);
|
if ((stack_used= available_stack_size(thd->thread_stack, &stack_used)) >= |
(long) (my_thread_stack_size - margin)) |
Attachments
Activity
Hello Eugene.
Thank you for your patch. Actually, we receive patches via Github Pull Requests: https://github.com/mariadb/server/pulls/
Sure, I've just made a pull request: https://github.com/MariaDB/server/pull/1424
As for the patch itself, well. When I tried gcc 9.2 with -flto A LOT of tests failed. MariaDB doesn't not work for me with LTO. Nobody uses it. I've tried clang with -flto=thin and that worked for me.
We as openSUSE (in openSUSE Tumbleweed) switched to usage of LTO with GCC by default some time ago. Build and test-suite works for me fine with GCC 9.
Please, share your experience with LTO. Did it work for you on gcc 9? Do you use MariaDB + LTO in production?
As mentioned, it works and all openSUSE TW users are now using mariadb with LTO. I can attach our build log, you may
see differences why it builds for us.
I don't like that fix much, it's trying to outsmart the compiler. I'd rather tell the compiler directly what we want, if that's possible. Would __attribute__((noinline)) help here? Or noclone or noipa? Or __attribute__((noinline,noclone,noipa)) ?
Well, one can definitely use -O0 pragma for the problematic function. On the other hand, I'm not
sure what set of noinline,noclone,noipa and portable for other compilers?
Is there a consensus that using -O0 will be accepted in a pull request?
Yes, I think both attributes and pragma could be accepted as a solution. Both are a way to tell the compiler what it should not optimize, both aren't portable, but it's ok, as the optimization issue is compiler specific anyway, so a compiler specific fix is fine here.
the last (https://github.com/MariaDB/server/pull/1424/commits/c1584b72097c385d2cb59d4e27c958168f78e0e6) commit looks ok, could you, please, push it?
Hi Martin!
Thank you for your patch. Actually, we receive patches via Github Pull Requests: https://github.com/mariadb/server/pulls/
As for the patch itself, well. When I tried gcc 9.2 with -flto A LOT of tests failed. MariaDB doesn't not work for me with LTO. Nobody uses it. I've tried clang with -flto=thin and that worked for me.
Please, share your experience with LTO. Did it work for you on gcc 9? Do you use MariaDB + LTO in production?