tune_serdes() falls through to next function apply_tx_lanes() in drivers/infiniband/hw/hfi1/platform.c

[tpgxyz]

Building kernel 5.3.1 with LLVM/clang-9.0.0 on x86_64

  CC [M]  drivers/infiniband/hw/hfi1/platform.o
BUILDSTDERR: ld.lld: warning: -r and --gc-sections may not be used together, disabling --gc-sections
BUILDSTDERR: ld.lld: warning: -r and --icf may not be used together, disabling --icf
BUILDSTDERR: drivers/infiniband/hw/hfi1/platform.o: warning: objtool: tune_serdes() falls through to next function apply_tx_lanes()
  CC [M]  drivers/gpu/drm/amd/amdgpu/../display/dc/clk_mgr/dcn10/rv1_clk_mgr.o

[nickdesaulniers]

Where is --icf added? (Identical Code Folding?)

[tpgxyz]

@nickdesaulniers This is enabled in ours lld by default
https://github.com/OpenMandrivaAssociation/llvm/blob/3e06cf7671e8abed54b1f48c9689810d3cbf59f3/lld-default-settings.patch#L61

[nickdesaulniers]

but you're also enabling --gc-sections by default: https://github.com/OpenMandrivaAssociation/llvm/blob/3e06cf7671e8abed54b1f48c9689810d3cbf59f3/lld-default-settings.patch#L32

So using your ld.lld with -r will always hit this issue. cc @MaskRay @rui314 @GeorgiiR @smithp35

[tpgxyz]

@nickdesaulniers Do you suggest to use ICF or gc-section not both at the same time ?

[nickdesaulniers]

Hmmm...I don't have the experience of maintaining a linux distro with a modified LLVM (AOSP LLVM has 1 out of tree patch at this point, which exports Clang's C++ API), but seeing so many changes makes me a little uncomfortable. In general, I appreciate you testing and reporting back your results. Another part of me thinks that it will be difficult to support something that's 99% llvm, but changes some defaults around.

Do you suggest to use ICF or gc-section not both at the same time ?

I recommend not modifying the default behavior of the compiler; if these flags are a net win for projects, then add the flags explicitly to those projects' build files, rather than relying on implicit defaults changed within the compiler. Otherwise, it becomes harder for upstream developers to reproduce issues you may encounter, and support falls back on you.

That said, I acknowledge how much work it would be to land such patches in each of the upstream codebases you distribute. Changing the compiler defaults is a quick win, but I'm not sure it's the ultimate long term solution.

[kees]

Should a distro-compiler-specific Label be added to categorize these?

[nickdesaulniers]

Should a distro-compiler-specific Label be added to categorize these?

I'm going to stand by discouraging such "value adds."

[dileks]

We will hit this again when [2] is upstream.

From my build-log and local patches.series:

$ grep "warning: objtool:" build-log_5.11.0-rc6-15-amd64-clang12-ias.txt 
drivers/infiniband/hw/hfi1/platform.o: warning: objtool: tune_serdes() falls through to next function apply_tx_lanes()

$ grep CET patches.series 
20bf2b378729 x86/build: Disable CET instrumentation in the kernel

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=x86/urgent
[2] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/urgent&id=20bf2b378729c4a0366a53e2018a0b70ace94bcd
[3] https://git.kernel.org/tip/tip/c/20bf2b378729c4a0366a53e2018a0b70ace94bcd

Update: Shorter link in [3] (points to the same commit as in [2])

[dileks]

Re-Opened as x86/build: Disable CET instrumentation in the kernel hit Linus tree.

Can someone confirm seeing the objtool-warning (again)?
Thanks.

[1] https://git.kernel.org/linus/20bf2b378729c4a0366a53e2018a0b70ace94bcd

[nathanchance]

$ make -skj"$(nproc)" LLVM=1 O=build/x86_64 distclean allyesconfig drivers/infiniband/hw/hfi1/platform.o

shows no objtool warnings on v5.11-rc7.

[dileks]

OK, thanks for the feedback.

I tried with:

--- a/drivers/infiniband/hw/hfi1/Makefile
+++ b/drivers/infiniband/hw/hfi1/Makefile

+CFLAGS_REMOVE_platform.o = -fcf-protection=none

But that still shows:

drivers/infiniband/hw/hfi1/platform.o: warning: objtool: tune_serdes() falls through to next function apply_tx_lanes()

Might be my linux-config (attached below) or one of my patches in my custom patchset (or used LLVM/Clang).

config-5.11.0-rc7-2-amd64-clang12-ias.txt

[dileks]

My make-line:

/usr/bin/perf_5.10 stat make V=1 -j4 LLVM=1 LLVM_IAS=1 PAHOLE=/opt/pahole/bin/pahole LOCALVERSION=-1-amd64-clang12-ias KBUILD_VERBOSE=1 KBUILD_BUILD_HOST=iniza KBUILD_BUILD_USER=sedat.dilek@gmail.com KBUILD_BUILD_TIMESTAMP=2021-02-07 bindeb-pkg KDEB_PKGVERSION=5.11.0~rc7-1~bullseye+dileks1

[dileks]

@jpoimboe

Uploaded gzip-ed drivers/infiniband/hw/hfi1/platform.o - can you look at it?

platform.o.gz

[jpoimboe]

Legit warning, maybe dead code or undefined behavior?

$ tools/objtool/objtool check --no-fp --backtrace /tmp/platform.o
/tmp/platform.o: warning: objtool: tune_serdes() falls through to next function apply_tx_lanes()
/tmp/platform.o: warning: objtool:   tune_serdes()+0x54f: (branch)
/tmp/platform.o: warning: objtool:   tune_serdes()+0x1bf: (branch)
/tmp/platform.o: warning: objtool:   tune_serdes()+0x7a: (branch)
/tmp/platform.o: warning: objtool:   tune_serdes()+0x0: <=== (sym)

     5a3:       80 bd 0b 14 00 00 03    cmpb   $0x3,0x140b(%rbp)
     5aa:       0f 85 8d 00 00 00       jne    63d <tune_serdes+0x10d>
     ...
     6e8:       83 f8 0a                cmp    $0xa,%eax
     6eb:       4c 89 24 24             mov    %r12,(%rsp)
     6ef:       0f 83 66 03 00 00       jae    a5b <tune_serdes+0x52b>
     ...
     a7c:       0f a3 c1                bt     %eax,%ecx
     a7f:       0f 83 8e 0e 00 00       jae    1913 <tune_serdes+0x13e3>
     ...
    1913:       66 2e 0f 1f 84 00 00    nopw   %cs:0x0(%rax,%rax,1)
    191a:       00 00 00
    191d:       0f 1f 00                nopl   (%rax)

0000000000001920 <apply_tx_lanes>:

[dileks]

Just as a note:
I have @jpoimboe objtool-vmlinux patchset on top of Linux v5.7-rc7 - that might explain why I see the objtool-warning and @nathanchance not on "vanilla" v5.7-rc7.

( BTW, is "vanilla" politically correct? What if someone likes both vanillaand chocko (thinking of ice-cream)? )
( Master is now main. )
( Raider chocolate is now called Twix in Germany. )
( Can foodstuff be political? "Eat or not to eat" is here the question? )
( /me moves to kitchen. )

[1] https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/log/?h=objtool/core

[arndb]

I created a reduced test case: https://godbolt.org/z/s69rsK

int b, f, m;
char *l;
__attribute__((__cold__)) int a(void);
int d(void);
int e(void);
int i(void);
int j(void);
int k(void);
static int c() {
  if (b)
    d();
  e();
  return 0;
}
int g(void) {
  int h = i();
  if (h)
    return h;
  h = j();
  if (h)
    return h;
  h = k();
  if (h)
    return h;
  h = k();
  if (h)
    if (f & 2)
      c();
  return 0;
}
int n(void) {
  char *cache = l;
  switch (cache[7] & 240 >> 4) {
  case 10 ... 11:
    return k();
  case 0 ... 9:
  case 12:
  case 14:
    return m;
  case 13:
  case 15:
    return a();
  }
  return 0;
}
void tune_serdes(void) { n(); }

Unfortunately, the output seems highly compiler specific and fragile, not sure if the output from godbolt.org has any problems.

With "clang-11 -O2", I get no objtool warning. With "clang-12 -O2", I get

platform.o: warning: objtool: tune_serdes()+0x30: can't find jump dest instruction at .text+0xdc"

with clang-13 I get:

platform.o: warning: objtool: n() falls through to next function tune_serdes()
platform.o: warning: objtool: tune_serdes() falls through to next function c()

[arndb]

For reference, this is the clang-13 -O2 assembler output:

        .globl  n                               # -- Begin function n
        .p2align        4, 0x90
        .type   n,@function
n:                                      # @n
        .cfi_startproc
# %bb.0:
        movq    l(%rip), %rax
        movzbl  7(%rax), %eax
        andl    $15, %eax
        cmpl    $10, %eax
        jae     .LBB1_1
.LBB1_6:
        movl    m(%rip), %eax
        retq
.LBB1_1:
        movl    $3072, %ecx                     # imm = 0xC00
        btl     %eax, %ecx
        jb      .LBB1_5
# %bb.2:
        movl    $20480, %ecx                    # imm = 0x5000
        btl     %eax, %ecx
        jb      .LBB1_6
# %bb.3:
        movl    $40960, %ecx                    # imm = 0xA000
        btl     %eax, %ecx
        jae     .LBB1_7
# %bb.4:
        jmp     a                               # TAILCALL
.LBB1_5:
        jmp     k                               # TAILCALL
.LBB1_7:
.Lfunc_end1:
        .size   n, .Lfunc_end1-n
        .cfi_endproc
                                        # -- End function
        .globl  tune_serdes                     # -- Begin function tune_serdes
        .p2align        4, 0x90
        .type   tune_serdes,@function
tune_serdes:                            # @tune_serdes
        .cfi_startproc
# %bb.0:
        movq    l(%rip), %rax
        movzbl  7(%rax), %eax
        andl    $15, %eax
        cmpl    $10, %eax
        jae     .LBB2_1
.LBB2_5:
        retq
.LBB2_1:
        movl    $3072, %ecx                     # imm = 0xC00
        btl     %eax, %ecx
        jb      .LBB2_6
# %bb.2:
        movl    $20480, %ecx                    # imm = 0x5000
        btl     %eax, %ecx
        jb      .LBB2_5
# %bb.3:
        movl    $40960, %ecx                    # imm = 0xA000
        btl     %eax, %ecx
        jae     .LBB2_4
# %bb.7:
        jmp     a                               # TAILCALL
.LBB2_6:
        jmp     k                               # TAILCALL
.LBB2_4:
.Lfunc_end2:
        .size   tune_serdes, .Lfunc_end2-tune_serdes
        .cfi_endproc

[dileks]

@arndb

All my Linux v5.11 Clang-IAS kernels have this issue.
LLVM/Clang v12.0.0-rc1 (dileks clang version 12.0.0 (https://github.com/llvm/llvm-project.git 8364f5369eeeb2da8db2bae7716c549930d8df93) or dileks clang version 13.0.0 (https://github.com/llvm/llvm-project.git c465429f286f50e52a8d2b3b39f38344f3381cce does not matter.

Yesterday, I added Clang-CFI to my custom patchset and do not see this warning.

I have archived all drivers/infiniband/hw/hfi1/platform.o files in [kBytes]:

164     5.11.0-1-amd64-clang12-ias/zstd-compressed/platform.o.zst
164     5.11.0-5-amd64-clang13-ias/zstd-compressed/platform.o.zst
276     5.11.0-7-amd64-clang13-cfi/zstd-compressed/platform.o.zst

If you want to inspect it - I can attach it here or send to you via email.
( Of course, I can send or attach my linux-config. )

NOTE: I disabled CONFIG_DEBUG_INFO_BTF as it is totally broken with Clang-CFI.

[nickdesaulniers]

I created a reduced test case: https://godbolt.org/z/s69rsK
For reference, this is the clang-13 -O2 assembler output:

Oof, yes we can see there's:

        ...
# %bb.3:
        ...
        jae     .LBB1_7
# %bb.4:
        jmp     a                               # TAILCALL
.LBB1_5:
        jmp     k                               # TAILCALL
.LBB1_7:

so a jump off the end of the function. Interestingly, I can add -mllvm -print-after-all -mllvm -print-module-scope -g0 to godbolt (I though I needed a Debug build of LLVM for that, perhaps godbolt is running a debug build, or I'm mistaken).

I see an unreachable get introduced into n, as the default destination of the switch; so LLVM has proven that all possible cases are covered (usually using expressions in the switch's value leads to such optimizations). Using -O2 -emit-llvm -S, we can see the unreachable is still there at the end of optimizations, so the unreachable gets lowered for codegen.

@zmodem do you know what pass usually cleans up such default destinations of switch's that are unreachable?

[nickdesaulniers]

still reproducible in godbolt, reminiscent of #621 (comment).

[nickdesaulniers]

https://reviews.llvm.org/D109103 looks like it should fix the case reported by @arndb above, which looks reproduced from the same code as this bugs description. The issue is reminiscent of #621, but https://reviews.llvm.org/D109103 doesn't fix #621. They are closely related; LLVM has 3 techniques it uses during instruction selection to lower switch like statements:

1. cases with contiguous ranges
2. bit tests
3. jump tables

This bug is about unreachable bit tests, while #621 seems to be about jump tables with entries to statically-proven unreachable blocks.