Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-21248

Test main.execution_constants fails with memory exhausted near

Details

    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

          Ascending order - Click to sort in descending order
          marxin Martin Liška created issue -
          marxin Martin Liška made changes -
          Field Original Value New Value
          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:

          ```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))
          ```           		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))
          }}
          marxin Martin Liška made changes -
          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))
          }}           		{{monospaced text}}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))
          }}
          marxin Martin Liška made changes -
          Description {{monospaced text}}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))
          }}           		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:


          {code:java}
          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))
          {code}
          kevg Eugene Kosov (Inactive) added a comment -

          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?

          kevg Eugene Kosov (Inactive) added a comment - 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?
          marxin Martin Liška added a comment - - edited

          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.

          marxin Martin Liška added a comment - - edited 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.
          marxin Martin Liška made changes -
          Attachment mariadb_log.txt.bz2 [ 49624 ]
          elenst Elena Stepanova made changes -
          Component/s Server [ 13907 ]
          Fix Version/s 10.5 [ 23123 ]
          Affects Version/s 10.5 [ 23123 ]
          Assignee Sergey Vojtovich [ svoj ]
          Labels contribution
          serg Sergei Golubchik added a comment -

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

          serg Sergei Golubchik added a comment - 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)) ?
          marxin Martin Liška added a comment - - edited

          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?

          marxin Martin Liška added a comment - - edited 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?
          marxin Martin Liška added a comment -

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

          marxin Martin Liška added a comment - Is there a consensus that using -O0 will be accepted in a pull request?
          marxin Martin Liška added a comment -

          May I please ping the issue?

          marxin Martin Liška added a comment - May I please ping the issue?
          serg Sergei Golubchik added a comment -

          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.

          serg Sergei Golubchik added a comment - 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.
          marxin Martin Liška added a comment -

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

          marxin Martin Liška added a comment - Good, I've just rebased the corresponding github PR.
          serg Sergei Golubchik added a comment - - edited

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

          serg Sergei Golubchik added a comment - - edited the last ( https://github.com/MariaDB/server/pull/1424/commits/c1584b72097c385d2cb59d4e27c958168f78e0e6 ) commit looks ok, could you, please, push it?
          serg Sergei Golubchik made changes -
          Assignee Sergey Vojtovich [ svoj ] Anel Husakovic [ anel ]
          anel Anel Husakovic made changes -
          Fix Version/s 10.5.1 [ 24029 ]
          Fix Version/s 10.5 [ 23123 ]
          Resolution Fixed [ 1 ]
          Status Open [ 1 ] Closed [ 6 ]
          serg Sergei Golubchik made changes -
          Workflow MariaDB v3 [ 101723 ] MariaDB v4 [ 157065 ]

          People

            anel Anel Husakovic
            marxin Martin Liška
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Git Integration

                Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.