Project

General

Profile

Bug #10469

GCC's -faggressive-loop-optimizations is too aggressive

Added by John Levon 8 months ago. Updated 8 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
Start date:
2019-02-27
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:

Description

Based on experience in OS-7511 with GCC optimizations eliding undefined but de facto correct code, a full build was performed with -fno-aggressive-loop-optimizations and then functions were compared to determine changes in text size. First, it should be said that most functions were not affected at all; of the 782027 functions, there were text size differences in only 2286 of them. Of these, text size increased in about half of them (1026). In investigating these, they seem to primarily be due to different register selection, often because the presence of the optimization seems to cause loop counters (and address calculation) to be replaced with addresses and address bounds.

Here are the functions for which -faggressive-loop-optimizations resulted in less program text and fewer backwards branches, ordered by fraction of text eliminated:

  #backwards branches, -faggressive-loop-optimizations ------------------------+
  #backwards branches, -fno-aggressive-loop-optimizations ----------------+    |
  #instructions, -faggressive-loop-optimizations --------------------+    |    |
  #instructions, -fno-aggressive-loop-optimizations ----------+      |    |    |
                                                              |      |    |    |
                                                              V      V    V    V
MOD`FUNC                                                  #INST O#INST  #BB O#BB
usr/lib/libpool.so.1`pool_resource_type_list                116     66    2    0
kernel/drv/amd64/xge`xgell_tx_close                         122     71    2    0
platform/i86pc/kernel/drv/amd64/dr`dr_bypass_device          82     50    1    0
usr/lib/libadutils.so.1`check_for_binary_attrs               80     51    1    0
kernel/drv/amd64/qede`qede_free_tx_bd_ring                  122     80    1    0
usr/sbin/i86/zdb`get_metaslab_refcount                      223    147    7    6
kernel/drv/amd64/qede`qede_alloc_io_structs                 166    112    2    1
usr/lib/amd64/libstandsaveargs.so`has_saved_fp              220    151    4    2
usr/lib/amd64/libsaveargs.so.1`has_saved_fp                 220    151    4    2
kernel/misc/amd64/kmdbmod`has_saved_fp                      220    151    4    2
kernel/drv/amd64/qede`ecore_int_disable_post_isr_release     67     46    1    0
usr/kernel/fs/amd64/lx_proc`lxpr_skip_mntopt                 91     63    1    0
usr/lib/libzpool.so.1`vdev_root_core_tvds                   172    121    5    4
usr/lib/amd64/gss/mech_krb5.so.1`rem_com_err_hook           187    135    4    3
usr/lib/smbsrv/amd64/libfksmbsrv.so.1`smb2_supported_version     95     71    1    0
usr/lib/amd64/libvarpd.so.1`libvarpd_prop_set               194    146    2    1
usr/lib/ndmp/ndmpd`ndmpd_make_exc_list                       78     59    1    0
kernel/fs/amd64/zfs`dnode_needs_remap                       252    191    3    1
kernel/drv/amd64/smrt`smrt_interrupts_free                  101     77    1    0
usr/lib/libzpool.so.1`dnode_needs_remap                     325    250    4    3
usr/lib/libzpool.so.1`vdev_count_leaves_impl                101     79    3    2
kernel/fs/amd64/zfs`dmu_objset_write_ready                  175    137    1    0
usr/lib/amd64/libzpool.so.1`dnode_needs_remap               309    245    4    2
usr/lib/amd64/libadutils.so.1`check_for_binary_attrs        114     92    1    0
kernel/drv/amd64/qede`qede_fastpath_config                  447    364    2    1
usr/kernel/drv/amd64/smbsrv`smb2_supported_version           87     71    1    0
usr/lib/libzpool.so.1`dmu_object_info_from_dnode            405    339    3    1
kernel/drv/amd64/emlxs`emlxs_fcoe_fcftab_fcfi_online_action    618    518    5    3
usr/sbin/i86/zdb`get_dtl_refcount                           140    118    3    2
usr/lib/amd64/libzfs_jni.so.1`zjni_get_default_property     283    241    4    3
usr/lib/mdb/kvm/amd64/fcp.so`targets_walk_i                 212    182    4    3
kernel/kmdb/amd64/fcp`targets_walk_i                        212    182    4    3
kernel/drv/amd64/qede`qede_init_locks                       341    293    2    1
kernel/drv/amd64/bnxe`elink_avoid_link_flap                 551    474    7    6
kernel/fs/amd64/zfs`dmu_object_info_from_dnode              323    280    2    1
usr/xpg6/bin/view`dhl_doit                                  242    210    4    2
usr/xpg6/bin/vi`dhl_doit                                    242    210    4    2
usr/xpg6/bin/vedit`dhl_doit                                 242    210    4    2
usr/xpg6/bin/expr`dhl_doit                                  242    210    4    2
usr/xpg6/bin/ex`dhl_doit                                    242    210    4    2
usr/xpg6/bin/edit`dhl_doit                                  242    210    4    2
usr/xpg6/bin/ed`dhl_doit                                    242    210    4    2
usr/xpg4/bin/view`dhl_doit                                  242    210    4    2
usr/xpg4/bin/vi`dhl_doit                                    242    210    4    2
usr/xpg4/bin/vedit`dhl_doit                                 242    210    4    2
usr/xpg4/bin/nl`dhl_doit                                    242    210    4    2
usr/xpg4/bin/expr`dhl_doit                                  242    210    4    2
usr/xpg4/bin/ex`dhl_doit                                    242    210    4    2
usr/xpg4/bin/edit`dhl_doit                                  242    210    4    2
usr/xpg4/bin/ed`dhl_doit                                    242    210    4    2
usr/bin/red`dhl_doit                                        242    210    4    2
usr/bin/nl`dhl_doit                                         242    210    4    2
usr/bin/expr`dhl_doit                                       242    210    4    2
usr/bin/ed`dhl_doit                                         242    210    4    2
kernel/drv/amd64/emlxs`emlxs_fcoe_fcftab_offline_timer      186    162    1    0
usr/lib/libzpool.so.1`dmu_objset_write_ready                397    346    5    3
lib/amd64/libnsl.so.1`getflag                               141    123    4    3
lib/amd64/libadm.so.1`puttext                              1833   1601   35   31
usr/sbin/iscsiadm`usage                                     270    237    7    4
usr/lib/amd64/libzpool.so.1`dmu_object_info_from_dnode      355    312    2    1
lib/libdhcputil.so.1`category_to_code                        89     79    2    1
lib/amd64/libcurses.so.1`waddwchnstr                        721    641   12    9
kernel/drv/amd64/nxge`nxge_update_rxdma_grp_properties     1440   1284   28   27
kernel/drv/amd64/nxge`nxge_cfg_verify_set                   655    585   17   15
kernel/drv/amd64/qede`qede_destroy_locks                    200    179    2    1
lib/libc.so.1`door_xcreate_n                               1271   1139   26   20
kernel/drv/amd64/qede`qede_fastpath_start_queues            824    739    6    3
usr/lib/libc/libc_hwcap3.so.1`door_xcreate_n               1245   1121   23   20
usr/lib/libc/libc_hwcap2.so.1`door_xcreate_n               1245   1121   23   20
usr/lib/libc/libc_hwcap1.so.1`door_xcreate_n               1245   1121   23   20
usr/lib/amd64/gss/mech_krb5.so.1`krb5_c_keyed_checksum_types    338    305    7    3
usr/lib/security/amd64/pkcs11_kernel.so.1`kernel_get_func_list    430    390    6    4
kernel/drv/amd64/mc-amd`iaddr_to_bank                       188    171    2    1
usr/lib/amd64/libzpool.so.1`dmu_objset_write_ready          354    322    3    2
lib/libnvpair.so.1`nvt_resize                               222    202    4    3
lib/libcurses.so.1`waddwchnstr                              713    649   18   16
kernel/drv/amd64/emlxs`emlxs_fcoe_fcftab_fcfi_offline_action    456    416    3    2
platform/i86xpv/kernel/amd64/unix`page_ctrs_cleanup         195    178    2    1
platform/i86pc/kernel/amd64/unix`page_ctrs_cleanup          195    178    2    1
usr/sbin/i86/zdb`dump_bpobj_subobjs                         355    325    5    4
kernel/drv/amd64/qede`ecore_hw_timers_stop_all               98     90    1    0
lib/libnsl.so.1`getflag                                      89     82    5    2
usr/lib/amd64/gss/mech_krb5.so.1`set_com_err_hook           118    109    1    0
kernel/drv/amd64/emlxs`emlxs_parse_prog_types              1633   1509   47   46
lib/amd64/libdhcputil.so.1`category_to_code                 108    100    2    1
lib/amd64/libzfs.so.1`zfeature_register                     222    206    2    1
usr/lib/krb5/libkadm5srv.so.1`kadm5_free_key_data           127    118    2    1
usr/lib/krb5/libkadm5clnt.so.1`kadm5_free_key_data          127    118    2    1
kernel/drv/amd64/ixgbe`ixgbe_chip_start                     345    321    5    4
kernel/drv/amd64/qede`qede_kstat_update                    1078   1004    4    3
kernel/fs/amd64/zfs`zfeature_register                       238    222    2    1
usr/kernel/fs/amd64/lx_proc`lxpr_open                       286    267    5    3
kernel/drv/amd64/xge`xgell_tx_open                          109    102    1    0
kernel/fs/amd64/zfs`vdev_raidz_reconstruct                  344    322   14   13
kernel/drv/amd64/nxge`nxge_update_txdma_properties         1478   1385   27   24
kernel/drv/amd64/nxge`nxge_update_rxdma_properties         1478   1385   27   24
kernel/drv/amd64/qede`qede_save_fp_dma_handles              479    449    4    3
kernel/drv/amd64/qede`qede_free_tx_ring_phys                130    122    1    0
usr/bin/amd64/nm`readsyms                                   776    729   15   14
kernel/strmod/amd64/ip`rule_prefix                          117    110    1    0
kernel/drv/amd64/ip`rule_prefix                             117    110    1    0
usr/bin/geniconvtbl`map_table_hash                         1720   1619   39   38
kernel/fs/amd64/udfs`ud_read_sparing_tbls                   327    308    4    2
usr/sbin/isns`setup_ref_lcp                                 437    412    7    5
platform/i86xpv/kernel/amd64/unix`page_ctrs_sz              581    548    9    8
platform/i86pc/kernel/amd64/unix`page_ctrs_sz               581    548    9    8
usr/bin/ftp`mypopen                                         480    453   11    9
usr/lib/libzpool.so.1`vdev_compact_children                 267    252    6    4
usr/lib/amd64/libtecla.so.1`gl_start_newline                220    208    3    2
lib/amd64/libnsl.so.1`expect_str                            303    287    3    1
usr/kernel/drv/amd64/ipf`ippr_rpcb_modv4                    537    509    4    2
kernel/misc/amd64/usba`usb_pipe_isoc_xfer                   422    400    6    5
usr/lib/amd64/gss/mech_krb5.so.1`reset_com_err_hook         117    111    1    0
usr/lib/gss/mech_krb5.so.1`krb5int_get_plugin_filenames     326    310    7    6
usr/lib/amd64/libldap.so.5`ldif_base64_encode_internal      502    478   12   11
usr/sbin/isnsadm`cvt_enumerate_rsp_to_get_req              1517   1445   31   30
usr/lib/amd64/libast.so.1`_ast_ftwalk                      1105   1057   27   24
usr/lib/nss_ldap.so.1`add_netgroup_name                     236    226    7    5
usr/lib/libsun_ima.so.1`isAuthMethodValid                    98     94    2    1
kernel/misc/amd64/nfssrv`nfslog_write_record                735    706    8    5
usr/lib/libdtrace.so.1`dt_fprintas                          128    123    3    2
usr/lib/ipf/amd64/ipftest`ippr_rpcb_modv4                   449    432    5    3
lib/libresolv.so.2`res_nsend                               2223   2139   32   31
usr/sbin/snoop`concat_args                                  213    205    6    5
kernel/drv/amd64/bnxe`BnxeHwReqPhyFlowSettings              493    475    5    4
usr/lib/libzpool.so.1`raidz_parity_verify                   367    354    6    5
usr/sbin/ipqosconf`get_originator_nm                         57     55    3    1
usr/ucb/groups`main                                         286    276    4    3
usr/bin/amd64/savecore`BZ2_hbMakeCodeLengths               1375   1327   26   25
kernel/amd64/genunix`BZ2_hbMakeCodeLengths                 1375   1327   26   25
platform/i86xpv/kernel/drv/amd64/xsvc`xsvc_devmap           774    747    8    7
usr/lib/libzfs_jni.so.1`zjni_get_default_property           324    313   10    9
platform/i86pc/kernel/drv/amd64/xsvc`xsvc_devmap            750    725    8    7
usr/lib/amd64/libsldap.so.1`__ns_ldap_trydoorcall_getfd     496    480    8    6
usr/bin/rdist`docmdargs                                     409    396    8    7
usr/kernel/fs/amd64/lx_proc`lxpr_doaccess                   519    503   14   13
lib/amd64/libc.so.1`__k_gconvert                            801    777   20   18
kernel/drv/amd64/yge`yge_resume                             541    525    7    6
platform/i86xpv/kernel/amd64/unix`hment_mapcnt              273    265    6    4
platform/i86pc/kernel/amd64/unix`hment_mapcnt               273    265    6    4
kernel/drv/amd64/qede`qede_fastpath_stop_queues             567    551    7    6
kernel/drv/amd64/si3124`si_watchdog_handler                 538    523   10    8
usr/lib/amd64/gss/mech_krb5.so.1`prof_hostnames2netaddrs    582    566   13   12
kernel/drv/amd64/rpcib`rib_svc_scq_handler                  329    320    5    4
kernel/drv/amd64/rpcib`rib_clnt_scq_handler                 329    320    5    4
kernel/misc/amd64/usba`usb_pipe_intr_xfer                   597    581   10    9
usr/sbin/iasl`LsFlushListingBuffer                          608    592    9    8
kernel/drv/amd64/ural`ural_set_chan                        1107   1078   11   10
kernel/misc/amd64/usba`usb_pipe_ctrl_xfer                   618    602   11    9
usr/bin/amd64/pflags`lwplook                                961    937   15   14
lib/amd64/libresolv.so.2`res_nsend                         2119   2071   31   29
kernel/drv/amd64/t4nex`t4_devo_detach                       819    801    9    8
usr/platform/i86pc/lib/fm/topo/plugins/x86pi.so`x86pi_bb_topparent    393    385    9    7
usr/bin/tail`lines                                         1033   1012   28   27
usr/sbin/isnsadm`cmdParse                                  1880   1842   34   32
usr/bin/lex`acompute                                        768    753   20   19
kernel/drv/amd64/qede`qede_alloc_tx_ring_phys               362    355    5    0
usr/bin/audiotest`uncompress_wave                          1379   1353   15   14
kernel/drv/amd64/dcam1394`dcam_ioctl                        850    834   19   17
usr/lib/amd64/libzpool.so.1`vdev_raidz_reconstruct          856    840   23   22
usr/bin/i86/ztest`make_vdev_root                            482    473    7    6
kernel/misc/amd64/pcmcia`SSSetSocket                        379    372    2    0
usr/sbin/stmfadm`cmdParse                                  2078   2040   34   32
usr/sbin/sbdadm`cmdParse                                   2078   2040   34   32
usr/sbin/sasinfo`cmdParse                                  2078   2040   34   32
usr/sbin/fcinfo`cmdParse                                   2078   2040   34   32
usr/sbin/fcadm`cmdParse                                    2078   2040   34   32
usr/demo/comstar/bin/aluaadm`cmdParse                      2078   2040   34   32
lib/svc/method/svc-stmf`cmdParse                           2078   2040   34   32
lib/svc/method/iscsi-target`cmdParse                       2078   2040   34   32
usr/lib/amd64/libzpool.so.1`zfs_blkptr_verify               938    921   19   18
kernel/drv/amd64/bnxe`BnxeHwReqPhyMediumSettings           3087   3032   43   41
usr/lib/amd64/libsldap.so.1`sortServerPref                  907    891   30   29
kernel/drv/amd64/yge`yge_detach                             455    447    2    1
kernel/misc/amd64/usba`usb_pipe_bulk_xfer                   467    459   10    8
usr/lib/libsldap.so.1`verify_value                          733    721   18   10
usr/lib/libc/libc_hwcap3.so.1`glob0                        1447   1424   66   62
usr/lib/libc/libc_hwcap3.so.1`glob0                        1447   1424   66   62
usr/lib/libc/libc_hwcap2.so.1`glob0                        1447   1424   66   62
usr/lib/libc/libc_hwcap2.so.1`glob0                        1447   1424   66   62
usr/lib/libc/libc_hwcap1.so.1`glob0                        1447   1424   66   62
usr/lib/libc/libc_hwcap1.so.1`glob0                        1447   1424   66   62
usr/lib/ipf/i86/ipftest`ippr_rpcb_modv4                     441    434    5    3
usr/sbin/amd64/lockstat`dprog_addevent                     1116   1100   11   10
usr/lib/gss/mech_krb5.so.1`rem_com_err_hook                 146    144    5    4
usr/bin/troff`casecs                                        146    144    3    1
usr/sbin/mpathadm`cmdParse                                 2585   2550   39   37
usr/bin/split`main                                         2954   2914   55   54
usr/sbin/iscsiadm`cmdParse                                 2623   2588   39   37
usr/lib/mdb/kvm/amd64/genunix.so`Pcred_gcore                309    305    3    2
kernel/drv/amd64/bnxe`ecore_config_vlan_mac                 391    386    8    5
usr/sbin/ndmpadm`ndmp_kill_sessions                         236    233    5    4
lib/amd64/libc.so.1`format_grouped_double                  1287   1271   17   16
usr/bin/pg`main                                            1384   1367   29   28
usr/lib/libzpool.so.1`dnode_sync                           2639   2607   44   42
lib/amd64/libtsol.so.2`__call_labeld                       1039   1027   13   12
usr/sbin/snoop`detail_attr_bitmap                           260    257    6    5
kernel/drv/amd64/vmxnet`Vxn_AllocInitBuffers                626    619    6    5
kernel/drv/amd64/mc-amd`rct_csintlv_bits                    359    355   10    9
usr/lib/mdb/kvm/amd64/genunix.so`pfile_callback            1546   1529   34   33
kernel/drv/amd64/yge`yge_attach                            1180   1168   18   14
usr/lib/libzpool.so.1`zfeature_register                     409    405    6    5
kernel/fs/amd64/zfs`zil_lwb_write_issue                     880    872    6    4
usr/lib/amd64/libsun_ima.so.1`setAuthMethods                346    343    4    2
kernel/drv/amd64/ixgbe`ixgbe_set_priv_prop                  822    815   23   22
kernel/kmdb/amd64/genunix`stacks                           3019   2995   55   51
usr/lib/mdb/proc/amd64/libc.so`stacks                      3051   3027   54   50
usr/lib/mdb/kvm/amd64/genunix.so`stacks                    3051   3027   54   50
usr/sbin/lofiadm`GetOptimum                                5338   5297   76   75
kernel/drv/amd64/bnxe`lm_cmng_init                         1429   1419   19   18
usr/lib/amd64/libzpool.so.1`zil_lwb_write_issue            1269   1261   11    9
kernel/kmdb/amd64/nca`nca_io2                              1273   1265   14   12
usr/lib/mdb/kvm/amd64/nca.so`nca_io2                       1313   1305   14   12
lib/amd64/libnsl.so.1`getipnodebyname                      1453   1445   31   29
usr/bin/i86/truss`report                                   1409   1402   22   21
usr/bin/lex`packtrans                                      1999   1990   47   44
usr/lib/zones/zonestatd`zsd_refresh_memory                 2708   2697   38   37
kernel/kmdb/amd64/genunix`pfile_callback                   1275   1270   33   32
kernel/drv/amd64/pcic`pcic_setup_adapter                   2602   2592   51   46
usr/bin/amd64/savecore`sendMTFValues                      12636  12598  117  116
kernel/amd64/genunix`sendMTFValues                        12768  12730  117  116
usr/lib/fm/amd64/libfmd_msg.so.1`fmd_msg_getitem_locked    1036   1033   13   12
usr/lib/zones/amd64/zoneadmd`add_net_for_linkid            1673   1669   20   19
lib/libc.so.1`__lc_collate_load                             880    878   14    5
usr/sbin/isnsadm`subUsage                                   920    918   14   13
usr/sbin/stmfadm`subUsage                                   944    942   14   13
usr/sbin/sbdadm`subUsage                                    944    942   14   13
usr/sbin/sasinfo`subUsage                                   944    942   14   13
usr/sbin/fcinfo`subUsage                                    944    942   14   13
usr/sbin/fcadm`subUsage                                     944    942   14   13
usr/demo/comstar/bin/aluaadm`subUsage                       944    942   14   13
lib/svc/method/svc-stmf`subUsage                            944    942   14   13
lib/svc/method/iscsi-target`subUsage                        944    942   14   13
kernel/drv/amd64/nv_sata`ck804_intr_process                1190   1188   21   17
usr/xpg6/bin/ls`pentry                                     2076   2074   37   36
usr/xpg4/bin/ls`pentry                                     2076   2074   37   36
usr/bin/ls`pentry                                          2076   2074   37   36

Of these, many (if not most) represent legitimate optimizations -- instances where the compiler unrolled a loop for which it can use the static size information to determine the number of iterations. For at least one class, though, -faggressive-loop-optimizations resulted in deeply incorrect code: any code iterating over dn_nblkptr in ZFS.

Take, for example, this code in dmu_objset_write_ready:

        for (int i = 0; i < dnp->dn_nblkptr; i++)
                bp->blk_fill += BP_GET_FILL(&dnp->dn_blkptr[i]);

Here is the generated code for this with -faggressive-loop-optimizations:

    dmu_objset_write_ready+0x27: f6 40 74 80        testb  $0x80,0x74(%rax)
    dmu_objset_write_ready+0x2b: ba 01 00 00 00     movl   $0x1,%edx
    dmu_objset_write_ready+0x30: 75 07              jne    +0x7 <dmu_objset_write_ready+0x39>
    dmu_objset_write_ready+0x32: 48 8b 90 98 00 00  movq   0x98(%rax),%rdx
                                 00 
    dmu_objset_write_ready+0x39: 48 89 53 58        movq   %rdx,0x58(%rbx)

Note: no loop! This code should look something more like:

    dmu_objset_write_ready+0x27: 48 8d 46 70        leaq   0x70(%rsi),%rax
    dmu_objset_write_ready+0x2b: 31 c9              xorl   %ecx,%ecx
    dmu_objset_write_ready+0x2d: 31 d2              xorl   %edx,%edx
    dmu_objset_write_ready+0x2f: 49 b8 00 00 00 00  movq   $0x8000000000,%r8
                                 80 00 00 00 
    dmu_objset_write_ready+0x39: 0f 1f 80 00 00 00  nopl   0x0(%rax)
                                 00 
    dmu_objset_write_ready+0x40: 4c 85 00           testq  %r8,(%rax)
    dmu_objset_write_ready+0x43: bf 01 00 00 00     movl   $0x1,%edi
    dmu_objset_write_ready+0x48: 75 04              jne    +0x4 <dmu_objset_write_ready+0x4e>
    dmu_objset_write_ready+0x4a: 48 8b 78 28        movq   0x28(%rax),%rdi
    dmu_objset_write_ready+0x4e: 48 01 f9           addq   %rdi,%rcx
    dmu_objset_write_ready+0x51: ff c2              incl   %edx
    dmu_objset_write_ready+0x53: 48 83 e8 80        subq   $-0x80,%rax  <0xffffffffffffff80>
    dmu_objset_write_ready+0x57: 48 89 4b 58        movq   %rcx,0x58(%rbx)
    dmu_objset_write_ready+0x5b: 0f b6 7e 03        movzbl 0x3(%rsi),%edi
    dmu_objset_write_ready+0x5f: 39 d7              cmpl   %edx,%edi
    dmu_objset_write_ready+0x61: 7f dd              jg     -0x23        <dmu_objset_write_ready+0x40>

The problem here is that dn_blkptr is used somewhat craftily; it is declared to be an array of size 1 (that is, appearing to be a flexible array member), but in fact is used to index into dn_bonus:

typedef struct dnode_phys {
        uint8_t dn_type;                /* dmu_object_type_t */
        uint8_t dn_indblkshift;         /* ln2(indirect block size) */
        uint8_t dn_nlevels;             /* 1=dn_blkptr->data blocks */
        uint8_t dn_nblkptr;             /* length of dn_blkptr */
        uint8_t dn_bonustype;           /* type of data in bonus buffer */
        uint8_t dn_checksum;            /* ZIO_CHECKSUM type */
        uint8_t dn_compress;            /* ZIO_COMPRESS type */
        uint8_t dn_flags;               /* DNODE_FLAG_* */
        uint16_t dn_datablkszsec;       /* data block size in 512b sectors */
        uint16_t dn_bonuslen;           /* length of dn_bonus */
        uint8_t dn_pad2[4];

        /* accounting is protected by dn_dirty_mtx */
        uint64_t dn_maxblkid;           /* largest allocated block ID */
        uint64_t dn_used;               /* bytes (or sectors) of disk space */

        uint64_t dn_pad3[4];

        blkptr_t dn_blkptr[1];
        uint8_t dn_bonus[DN_MAX_BONUSLEN - sizeof (blkptr_t)];
        blkptr_t dn_spill;
} dnode_phys_t;

The ZoL folks also ran into this ; their fix was to compile with -fno-aggressive-loop-optimizations -- but they ultimately fixed the structure and then re-enabled the optimization . While this structure should definitely be changed to give the compiler a better idea of what's going on, it feels too risky to leave -faggressive-loop-optimizations on (it is the default with -O2). The win it delivers seems far too modest to pay for the risk of potential data corruption induced by incorrect code generation; we should compile with -fno-aggressive-loop-optimizations.

We should disable this dangerous option across illumos.

This bug mirrors Joyent upstream bug OS-7517


Related issues

Related to illumos gate - Feature #9996: use GCC 7 as default primary compilerClosed2018-11-21

Actions

History

#1

Updated by John Levon 8 months ago

#2

Updated by John Levon 8 months ago

  • Related to Feature #9996: use GCC 7 as default primary compiler added
#3

Updated by Joshua M. Clulow 8 months ago

  • Description updated (diff)
  • Tags deleted (needs-triage)
#4

Updated by Joshua M. Clulow 8 months ago

Additional comments from original ticket:

Bryan Cantrill added a comment - 16/Jan/19 11:42 AM

Two additional notes. First, the difference in has_saved_fp is actually due to an array over-read; see OS-7518 (#10470). Second, vdev_raidz_reconstruct (which troublingly appears in the list of potentially suspect functions) appears to have correct code generated with -faggressive-loop-optimizations, and is in that regard apparently a false positive.

John Levon added a comment - 17/Jan/19 2:06 AM

Previously we thought that GCC would do this upon 1-sized last-member arrays (not just the dn_blkptr grossness). Was that mistaken, or is that still potentially the case? The reason I ask is that the GCC source in this area has special code for this, implying that if it is doing bad things, it would actually be considered a GCC Bug.

Bryan Cantrill added a comment - 22/Jan/19 9:11 AM

It appears that GCC is indeed handling the 1-sized last-member arrays properly (fortunately). The code generated is quite different (which led to the concern that loops were being obviated), but having gone over some of these functions (e.g. vdev_raidz_reconstruct) quite carefully, it seems that the code is correct in this case, even in the presence of -faggressive-loop-optimizations.

#5

Updated by Electric Monk 8 months ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 92c1a61163ff6a0655b27bd429856e171e7ce5f5

commit  92c1a61163ff6a0655b27bd429856e171e7ce5f5
Author: Bryan Cantrill <bryan@joyent.com>
Date:   2019-03-01T11:11:54.000Z

    10468 __ctype_mask[EOF] has been working by accident
    10469 GCC's -faggressive-loop-optimizations is too aggressive
    10470 array over-read in has_saved_fp()
    Reviewed by: Robert Mustacchi <rm@joyent.com>
    Reviewed by: John Levon <john.levon@joyent.com>
    Reviewed by: Toomas Soome <tsoome@me.com>
    Reviewed by: Igor Kozhukhov <igor@dilos.org>
    Reviewed by: Andy Fiddaman <andy@omniosce.org>
    Reviewed by: Gergő Doma <domag02@gmail.com>
    Reviewed by: Gary Mills <gary_mills@fastmail.fm>
    Approved by: Richard Lowe <richlowe@richlowe.net>

Also available in: Atom PDF