Project

General

Profile

Actions

Feature #14282

closed

Warn when an inline function isn't

Added by Andy Fiddaman 6 months ago. Updated 5 months ago.

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

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

While comparing the generated code from a gcc7 primary compiler to that from gcc10, it became apparent that several functions marked explicitly with inline were not being inlined at all, and that gcc10 inlined even fewer due to changes in the threshold for considering a function too large to inline.

I've gone through and cleaned up the few functions that can never be inlined for various reasons, enabled the compiler's warning option that will report when a function is not inlined, and explicitly specified the function size limit to get more consistent behaviour between the gcc versions.


Related issues

Related to illumos gate - Feature #14421: use GCC 10 as default primary compilerIn Progress

Actions
Actions #1

Updated by Electric Monk 6 months ago

  • Gerrit CR set to 1836
Actions #2

Updated by Andy Fiddaman 5 months ago

In addition to the checks I've done to confirm that objects from a gcc 10 primary build are now much closer to those from gcc 7, I ran a wsdiff comparing before and after the associated patch with gcc7 (as that's the current primary compiler).

usr/sbin/iasl
usr/sbin/acpixtract
usr/sbin/acpidump
etc/versions/build
etc/motd
platform/i86pc/kernel/amd64/unix
platform/i86xpv/kernel/amd64/unix

These changes are a result of the git commit hash changing between builds (since I cherry picked the change).

usr/lib/smbsrv/amd64/libfksmbsrv.so.1
usr/lib/smbsrv/libfksmbsrv.so.1
usr/lib/smbfs/fksmbcl
usr/lib/smbfs/amd64/libfknsmb.so.1
usr/lib/smbfs/libfknsmb.so.1
usr/lib/smbfs/amd64/libfksmbfs.so.1
usr/lib/smbfs/libfksmbfs.so.1

opt/os-tests/tests/cores/amd64/libdumper.so.1
opt/os-tests/tests/cores/dumper.64
opt/os-tests/tests/cores/dumper.32
opt/os-tests/tests/cores/libdumper.so.1

Changes in .debug_{str,info} sections due to differing gcc options.

usr/bin/raidz_test

The ilog2() function has changed as follows. The change moved this function from raidz_test.h to raidz_test.c and removes the inline keyword (it was never inlined due to recursion). This has obviously changed the code generation in some way, but it still looks right, just slightly less optimised for the terminal case. I ran some ZFS tests that use the raidz_test binary and they succeeded.

ilog2()
-    ilog2:                 31 c0              xorl   %eax,%eax
-    ilog2+0x2:             48 83 ff 01        cmpq   $0x1,%rdi
-    ilog2+0x6:             77 08              ja     +0x8      <ilog2+0x10>
-    ilog2+0x8:             f3 c3              repz ret
-    ilog2+0xa:             66 0f 1f 44 00 00  nopw   0x0(%rax,%rax)
-    ilog2+0x10:            48 89 f8           movq   %rdi,%rax
-    ilog2+0x13:            48 d1 e8           shrq   $0x1,%rax
-    ilog2+0x16:            48 83 f8 01        cmpq   $0x1,%rax
-    ilog2+0x1a:            74 ec              je     -0x14     <ilog2+0x8>
-    ilog2+0x1c:            55                 pushq  %rbp
-    ilog2+0x1d:            48 c1 ef 02        shrq   $0x2,%rdi
-    ilog2+0x21:            48 89 e5           movq   %rsp,%rbp
-    ilog2+0x24:            e8 d7 ff ff ff     call   -0x29     <ilog2>
-    ilog2+0x29:            5d                 popq   %rbp
-    ilog2+0x2a:            48 83 c0 02        addq   $0x2,%rax
-    ilog2+0x2e:            c3                 ret
-    .text+0x1cf:           90                 nop
+    ilog2:                 48 83 ff 01        cmpq   $0x1,%rdi
+    ilog2+0x4:             77 0a              ja     +0xa      <ilog2+0x10>
+    ilog2+0x6:             31 c0              xorl   %eax,%eax
+    ilog2+0x8:             c3                 ret
+    ilog2+0x9:             0f 1f 80 00 00 00  nopl   0x0(%rax)
+                           00
+    ilog2+0x10:            55                 pushq  %rbp
+    ilog2+0x11:            48 d1 ef           shrq   $0x1,%rdi
+    ilog2+0x14:            48 89 e5           movq   %rsp,%rbp
+    ilog2+0x17:            e8 e4 ff ff ff     call   -0x1c     <ilog2>
+    ilog2+0x1c:            5d                 popq   %rbp
+    ilog2+0x1d:            48 ff c0           incq   %rax
+    ilog2+0x20:            c3                 ret

lib/libld.so.4
lib/amd64/libld.so.4

The is_name_cmp() function is no longer being inlined by gcc7.
gcc10 will never inline this and reports inlining failed in call to 'is_name_cmp': call is unlikely and code size would grow

lib/libsqlite-native.o

This is another object which is built and left containing DWARF. The wsdiff differences are due to the differing gcc options used.
Of note, libsqlite-sys.so.2.8.15 is not reported by wsdiff.

Actions #3

Updated by Andy Fiddaman 5 months ago

The initial scope of this change was intended to be all of illumos-gate, but during testing it was found that this flagged up warnings in the userland parts due to pulling in out-of-gate components such as perl and openssl.

In the end, the warnings were only enabled for uts/ and changes were limited to the same area plus a few small parts of userland that were highlighted by gcc10's checks.

Actions #4

Updated by Electric Monk 5 months ago

  • Status changed from In Progress to Closed
  • % Done changed from 0 to 100

git commit fd5e5f438a15881f651587df3e961609eb00778d

commit  fd5e5f438a15881f651587df3e961609eb00778d
Author: Andy Fiddaman <omnios@citrus-it.co.uk>
Date:   2022-01-13T00:08:12.000Z

    14282 Warn when an inline function isn't
    Reviewed by: Rich Lowe <richlowe@richlowe.net>
    Reviewed by: Toomas Soome <tsoome@me.com>
    Approved by: Dan McDonald <danmcd@joyent.com>

Actions #5

Updated by Andy Fiddaman 4 months ago

  • Related to Feature #14421: use GCC 10 as default primary compiler added
Actions

Also available in: Atom PDF