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

gcc 12/13: -Warray-bounds when dereferencing value returned from TABLE_SHARE::db_type()

Details

    Description

      See
      <https://buildbot.mariadb.net/buildbot/builders/kvm-deb-bookworm-amd64/builds/1122/steps/compile/logs/warnings%20%28214%29>:

      /home/buildbot/buildbot/build/mariadb-11.4.5/sql/sql_base.cc: In function ‘my_bool mysql_rm_tmp_tables()’:
      /home/buildbot/buildbot/build/mariadb-11.4.5/sql/sql_base.cc:9435:30: warning: array subscript 0 is outside array bounds of ‘handlerton [0]’ [-Warray-bounds]
       9435 |             share.db_type()->drop_table(share.db_type(), path_copy);
            |             ~~~~~~~~~~~~~~~~~^~~~~~~~~~
      ...
      /home/buildbot/buildbot/build/mariadb-11.4.5/sql/sql_partition.cc: In function ‘bool handle_list_of_fields(THD*, List_iterator<const char>, TABLE*, partition_info*, bool)’:
      /home/buildbot/buildbot/build/mariadb-11.4.5/sql/sql_partition.cc:720:32: warning: array subscript 0 is outside array bounds of ‘handlerton [0]’ [-Warray-bounds]
        720 |       if (table->s->db_type()->partition_flags &&
            |           ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
      In file included from /home/buildbot/buildbot/build/mariadb-11.4.5/sql/mariadb.h:29,
                       from /home/buildbot/buildbot/build/mariadb-11.4.5/sql/sql_partition.cc:50:
      /home/buildbot/buildbot/build/mariadb-11.4.5/sql/sql_partition.cc: In function ‘bool fix_partition_func(THD*, TABLE*, bool)’:
      /home/buildbot/buildbot/build/mariadb-11.4.5/sql/sql_partition.cc:2056:40: warning: array subscript 0 is outside array bounds of ‘handlerton [0]’ [-Warray-bounds]
       2056 |   if (unlikely((!(table->s->db_type()->partition_flags &&
            |                   ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
      /home/buildbot/buildbot/build/mariadb-11.4.5/include/my_global.h:365:44: note: in definition of macro ‘unlikely’
        365 | #define unlikely(x)     __builtin_expect(((x) != 0),0)
            |                                            ^
      /home/buildbot/buildbot/build/mariadb-11.4.5/sql/sql_partition.cc: In function ‘bool partition_key_modified(TABLE*, const MY_BITMAP*)’:
      /home/buildbot/buildbot/build/mariadb-11.4.5/sql/sql_partition.cc:2786:28: warning: array subscript 0 is outside array bounds of ‘handlerton [0]’ [-Warray-bounds]
       2786 |   if (table->s->db_type()->partition_flags &&
            |       ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
      

      Locally it requires at least -O3 in CMAKE_CXX_FLAGS to reproduce.
      Triggered even if we replace plugin_hton(db_plugin) with
      (handlerton *)((*db_plugin)->data) in db_type(). Therefore it
      looks like a false positive (or
      https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104828) to me.

      I was wondering whether -O3 causes Warray-bounds=2, but the warning
      also appears with -O3 -Warray-bounds=1.

      gcc versions:
      Mine:

      $ g++ --version
      g++ (Debian 13.2.0-13) 13.2.0

      In the buildbot stdout:

      g++ (Debian 12.2.0-14) 12.2.0

      Attachments

        Activity

          ycp Yuchen Pei added a comment -

          Hi danblack, ptal thanks:

          0432a765de0 upstream/bb-10.5-mdev-35840 MDEV-35840 Eliminate -warray-bounds triggered by TABLE_SHARE::db_type()
          

          Old buildbot pages:

          https://buildbot.mariadb.net/buildbot/changes/808821
          https://buildbot.mariadb.net/buildbot/changes/808823

          ycp Yuchen Pei added a comment - Hi danblack , ptal thanks: 0432a765de0 upstream/bb-10.5-mdev-35840 MDEV-35840 Eliminate -warray-bounds triggered by TABLE_SHARE::db_type() Old buildbot pages: https://buildbot.mariadb.net/buildbot/changes/808821 https://buildbot.mariadb.net/buildbot/changes/808823
          marko Marko Mäkelä added a comment - - edited

          I’m glad to see some progress here. The following is reducing some but not all repetition:

          diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc
          index 6228991d46c..73742a1da44 100644
          --- a/sql/sql_partition.cc
          +++ b/sql/sql_partition.cc
          @@ -721,9 +721,10 @@ static bool handle_list_of_fields(THD *thd, List_iterator<const char> it,
               }
               else
               {
          -      if (table->s->db_type()->partition_flags &&
          -          (table->s->db_type()->partition_flags() & HA_USE_AUTO_PARTITION) &&
          -          (table->s->db_type()->partition_flags() & HA_CAN_PARTITION))
          +      handlerton *ht= table->s->db_type();
          +      if (ht->partition_flags &&
          +          (ht->partition_flags() & HA_USE_AUTO_PARTITION) &&
          +          (ht->partition_flags() & HA_CAN_PARTITION))
                 {
                   /*
                     This engine can handle automatic partitioning and there is no
          

          We could test both flags at once with a single call and a single bitwise-and operation:

          if (ht->partition_flags &&
              !~(ht->partition_flags() & (HA_USE_AUTO_PARTITION | HT_CAN_PARTITION)))
          

          Also, it seems that the ternary operator could be used here:

          @@ -4927,10 +4932,11 @@ uint prep_alter_part_table(THD *thd, TABLE *table, Alter_info *alter_info,
                    if default partitioning is used.
                 */
           
          +      handlerton *ht= table->s->db_type();
                 if (tab_part_info->part_type != HASH_PARTITION ||
          -          ((table->s->db_type()->partition_flags() & HA_USE_AUTO_PARTITION) &&
          +          ((ht->partition_flags() & HA_USE_AUTO_PARTITION) &&
                      !tab_part_info->use_default_num_partitions) ||
          -          ((!(table->s->db_type()->partition_flags() & HA_USE_AUTO_PARTITION)) &&
          +          ((!(ht->partition_flags() & HA_USE_AUTO_PARTITION)) &&
                      tab_part_info->use_default_num_partitions))
                 {
                   my_error(ER_REORG_NO_PARAM_ERROR, MYF(0));
          

          That is, (a && b) || (!a || c) could be better written as a ? b : c, to avoid evaluating a twice. In fact, here we would seem to have c = !b and therefore this would seem to be an equivalence operation. While the bitwise exclusive-or operator ^ has no logical counterpart (double caret), the equality comparison == comes close, that is:

          !(table->s->db_type()->partition_flags() & HA_USE_AUTO_PARTITION) ==
          !!tab_part_info->use_default_num_partitions
          

          The double negation !! will normalize 0 to false and nonzero to true.

          marko Marko Mäkelä added a comment - - edited I’m glad to see some progress here. The following is reducing some but not all repetition: diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc index 6228991d46c..73742a1da44 100644 --- a/sql/sql_partition.cc +++ b/sql/sql_partition.cc @@ -721,9 +721,10 @@ static bool handle_list_of_fields(THD *thd, List_iterator<const char> it, } else { - if (table->s->db_type()->partition_flags && - (table->s->db_type()->partition_flags() & HA_USE_AUTO_PARTITION) && - (table->s->db_type()->partition_flags() & HA_CAN_PARTITION)) + handlerton *ht= table->s->db_type(); + if (ht->partition_flags && + (ht->partition_flags() & HA_USE_AUTO_PARTITION) && + (ht->partition_flags() & HA_CAN_PARTITION)) { /* This engine can handle automatic partitioning and there is no We could test both flags at once with a single call and a single bitwise-and operation: if (ht->partition_flags && !~(ht->partition_flags() & (HA_USE_AUTO_PARTITION | HT_CAN_PARTITION))) Also, it seems that the ternary operator could be used here: @@ -4927,10 +4932,11 @@ uint prep_alter_part_table(THD *thd, TABLE *table, Alter_info *alter_info, if default partitioning is used. */ + handlerton *ht= table->s->db_type(); if (tab_part_info->part_type != HASH_PARTITION || - ((table->s->db_type()->partition_flags() & HA_USE_AUTO_PARTITION) && + ((ht->partition_flags() & HA_USE_AUTO_PARTITION) && !tab_part_info->use_default_num_partitions) || - ((!(table->s->db_type()->partition_flags() & HA_USE_AUTO_PARTITION)) && + ((!(ht->partition_flags() & HA_USE_AUTO_PARTITION)) && tab_part_info->use_default_num_partitions)) { my_error(ER_REORG_NO_PARAM_ERROR, MYF(0)); That is, (a && b) || (!a || c) could be better written as a ? b : c , to avoid evaluating a twice. In fact, here we would seem to have c = !b and therefore this would seem to be an equivalence operation. While the bitwise exclusive-or operator ^ has no logical counterpart (double caret), the equality comparison == comes close, that is: !(table->s->db_type()->partition_flags() & HA_USE_AUTO_PARTITION) == !!tab_part_info->use_default_num_partitions The double negation !! will normalize 0 to false and nonzero to true .
          ycp Yuchen Pei added a comment - - edited

          Thanks for the review comments marko and danblack. Updated the commit - ptal thanks:

          5ef712bdced bb-10.5-mdev-35840 MDEV-35840 Eliminate -warray-bounds triggered by TABLE_SHARE::db_type()
          

          ycp Yuchen Pei added a comment - - edited Thanks for the review comments marko and danblack . Updated the commit - ptal thanks: 5ef712bdced bb-10.5-mdev-35840 MDEV-35840 Eliminate -warray-bounds triggered by TABLE_SHARE::db_type()

          For clarification: I had misplaced the parenthesis in my previous suggestion:

          if (ht->partition_flags &&
              !(~ht->partition_flags() & (HA_USE_AUTO_PARTITION | HT_CAN_PARTITION)))
          

          danblack suggested the more readable variant

          if (ht->partition_flags &&
              (ht->partition_flags() & (HA_USE_AUTO_PARTITION | HT_CAN_PARTITION)) ==
              (HA_USE_AUTO_PARTITION | HT_CAN_PARTITION)))
          

          According to https://godbolt.org, only starting with clang 13 or GCC 13 targeting AMD64 at -O2 level they translate to the equivalent code. Older compiler versions actually emit code for both the bitwise and and the equality cmp. Later ones emit not and test, which my suggestion would always compile to. In any case, either variant is better than the original, which seems to have duplicated the function call.

          marko Marko Mäkelä added a comment - For clarification: I had misplaced the parenthesis in my previous suggestion: if (ht->partition_flags && !(~ht->partition_flags() & (HA_USE_AUTO_PARTITION | HT_CAN_PARTITION))) danblack suggested the more readable variant if (ht->partition_flags && (ht->partition_flags() & (HA_USE_AUTO_PARTITION | HT_CAN_PARTITION)) == (HA_USE_AUTO_PARTITION | HT_CAN_PARTITION))) According to https://godbolt.org , only starting with clang 13 or GCC 13 targeting AMD64 at -O2 level they translate to the equivalent code. Older compiler versions actually emit code for both the bitwise and and the equality cmp . Later ones emit not and test , which my suggestion would always compile to. In any case, either variant is better than the original, which seems to have duplicated the function call.
          danblack Daniel Black added a comment -

          Looks good ycp, good to merge.

          danblack Daniel Black added a comment - Looks good ycp , good to merge.
          ycp Yuchen Pei added a comment -

          thanks for the review - pushed 78157c4765f2c086fabe183d51d7734ecffdbdd8 to 10.5

          ycp Yuchen Pei added a comment - thanks for the review - pushed 78157c4765f2c086fabe183d51d7734ecffdbdd8 to 10.5

          People

            ycp Yuchen Pei
            ycp Yuchen Pei
            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.