[MDEV-21248] Test main.execution_constants fails with memory exhausted near Created: 2019-12-08  Updated: 2020-02-08  Resolved: 2020-02-08

Status: Closed
Project: MariaDB Server
Component/s: Server
Affects Version/s: 10.5
Fix Version/s: 10.5.1

Type: Bug Priority: Major
Reporter: Martin Liška Assignee: Anel Husakovic
Resolution: Fixed Votes: 0
Labels: contribution

Attachments: File mariadb_log.txt.bz2    

 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))



 Comments   
Comment by Eugene Kosov (Inactive) [ 2019-12-08 ]

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?

Comment by Martin Liška [ 2019-12-08 ]

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.

Comment by Sergei Golubchik [ 2020-01-01 ]

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)) ?

Comment by Martin Liška [ 2020-01-02 ]

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?

Comment by Martin Liška [ 2020-01-09 ]

Is there a consensus that using -O0 will be accepted in a pull request?

Comment by Martin Liška [ 2020-01-20 ]

May I please ping the issue?

Comment by Sergei Golubchik [ 2020-01-21 ]

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.

Comment by Martin Liška [ 2020-01-22 ]

Good, I've just rebased the corresponding github PR.

Comment by Sergei Golubchik [ 2020-02-05 ]

the last (https://github.com/MariaDB/server/pull/1424/commits/c1584b72097c385d2cb59d4e27c958168f78e0e6) commit looks ok, could you, please, push it?

Generated at Thu Feb 08 09:05:42 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.