ICP: Add missing stack frame info to SHA asm files

[AttilaFueloep]

Motivation and Context

Since the assembly routines calculating SHA checksums don't use a standard stack layout, CFI directives are needed to unroll the
stack.

Description

Add the needed directives.

How Has This Been Tested?

Just a local build.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[sempervictus]

Will try to get this into a build cycle over the weekend. What're the chances of it eating my pets and setting my fridge aflame?

[ahrens]

Does this address #9394 ?

[youzhongyang]

I am quite confused. The push %rbx is still the first one pushing %rbx to the stack. Normally, %rbp should be the first register to be saved on the stack by any x86-64 ABI conformant function.

Please refer to https://blogs.oracle.com/sherrym/obtaining-function-arguments-on-amd64.

[AttilaFueloep]

@sempervictus I'd say that this is a low risk change. The code itself only changed slightly, most of the stuff just adds DWARF info to the object file. Nonetheless it's always good to run on a test system first.

[AttilaFueloep]

@ahrens Can't tell for sure. It depends if the linux tooling is making use of the DWARF info contained in the kernel modules. If you have an easy way to test, that would be great.

[AttilaFueloep]

@youzhongyang pushing %rbp first is only part of the convention, more importantly the current %rbp must also point to the previous %rbp on the stack all the time (frame pointer). That way you can unroll the stack by repeatedly dereferencing %rbp. As I wrote in #11728 this code uses %rbp as an general purpose register and there is no spare register left, meaning there is no way to achieve that. So that code behaves like gcc with -fomit-frame-pointer. To work around that problem there are DWARF features tooling and debuggers can use to follow the stack anyhow (CFA: Canonical Frame Address). My knowledge is that the linux tooling makes use of that DWARF feature, but only testing can confirm that. So if you have a way to test if this PR fixes your problem, that would be great.

[AndyLavr]

Build result with applied patch "ICP: Add missing stack frame info to SHA asm files":

  CC [M]  drivers/iio/pressure/st_pressure_spi.o
  AR [M]  drivers/iio/pressure/bmp280.o
  AR [M]  drivers/iio/industrialio.o
  AR [M]  drivers/iio/pressure/st_pressure.o
  GEN     .version
  CHK     include/generated/compile.h
  GEN     .tmp_initcalls.lds
  GEN     .tmp_symversions.lds
  LTO     vmlinux.o
  OBJTOOL vmlinux.o
vmlinux.o: warning: objtool: .text+0x4b66: can't find jump dest instruction at .text+0x4ba0
  MODPOST vmlinux.symvers
  MODINFO modules.builtin.modinfo
  GEN     modules.builtin
  LD      .tmp_vmlinux.kallsyms1
  KSYMS   .tmp_vmlinux.kallsyms1.S

See the fix here - STACK_FRAME_NON_STANDARD

[AttilaFueloep]

Yes, the data in the text section. Not sure about the performance impacts, if any. Caching comes to mind.

[AttilaFueloep]

I should add that compilation as kernel module has the appropriate OBJECT_FILES_NON_STANDARD_filename.o in place in the Makefile. This looks like an issue with in tree compilation, right?

[AndyLavr]

@AttilaFueloep

OBJECT_FILES_NON_STANDARD_filename.o in place in the Makefile

For build with Clang + LTO, this doesn't work:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Makefile.build?h=v5.12-rc2#n220

[AndyLavr]

@AttilaFueloep

Yes, the data in the text section.

This is wrong, they should be on .data or .rodata.

Not sure about the performance impacts, if any. Caching comes to mind.

No performance issues were noticed.

[behlendorf]

Could someone clarify where this PR stands? From the comments it sounds like this was largely blocked by the need for additional testing and confirmation that it resolved the warnings. Are there any remaining correctness concerns?

[behlendorf]

Since this should be a low risk change, and no issues with it have been reported, I'd like to move forward with getting it merged. Unless there are remaining concerns let's plan on merging it next week.

[youzhongyang]

Is there any reason %rbp can't be pushed first? that is all I asked.

[AttilaFueloep]

Sorry for the delay, been away from a keyboard for quite a while.

@behlendorf IRC the tests @AndyLavr did were confirming. Unfortunately the reproducer given in #9394 didn't work in my setup, so I can't tell if this will fix the warning there. I also failed to reproduce the warnings given in #11728, so the same holds for #11728 as well. At least this change will allow gdb to unwind the stack properly, which should be a win given the low risk nature of the change. So I'd argue: "It won't hurt to merge."

@youzhongyang Well, pushing %rpb first won't buy you anything, I tried to explain this above.