Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make failed with clang built kernel 6.4.0 #67

Open
mzyx-hnu opened this issue Sep 26, 2023 · 4 comments
Open

Make failed with clang built kernel 6.4.0 #67

mzyx-hnu opened this issue Sep 26, 2023 · 4 comments

Comments

@mzyx-hnu
Copy link

Make failed when installing with DKMS in a clang built kernel 6.4.0 environment. A "warning: stack frame size (420) exceeds limit (400) in 'func' [-Wframe-larger-than][-Werror] " occorred.

The root cause is clang spent more than 400bytes of stack frame, which triggered -Werror, -Wframe-larger-than=400 and failed. I checked both source code and found that gcc will only calculates the size of temporary variables in the code as the stack frame size. In contrast, clang calculates more objects such as stack pointer offsets, stack space allocated by alloc() func. So clang gets larger and more accurate results.

I don't understand the reason behind this 400bytes restriction, but since clang outputs a larger stack frame with the same code, is it reasonable to make a separate stack frame size limite for clang built kernel?

@raeburn
Copy link
Member

raeburn commented Sep 26, 2023

The intent was to help avoid stack overflows in cases where some of our call stacks may go many layers deep. (I believe the kernel stack sizes have increased since we introduced the check, but they’re still not huge and having such a sanity check isn’t the worst thing.) There’s no magic number that’s going to make the code right or wrong; the 400 byte limit is pretty arbitrary, but helped us catch some of the biggest offenders. Any cases of functions with large local allocations get reviewed to see if static or heap storage would work well.

I wasn’t aware of the difference in how clang evaluates the frame size for the warning. Thank you for that analysis. We do builds against clang in our automated testing but most of our development uses gcc. Hmm... actually it looks like our only clang builds use kernel snapshots, our latest working snapshot is from a week ago. Also, we’re using the clang compiler distributed with Fedora 37, so if that’s out of date, a newer one might do its stack allocations or its frame size calculations differently.

Yes, I think it would make sense to consider different limits based on the compiler, in light of their different calculations. I haven’t checked but I think it should be easy to do in the makefile.

However, it may also make sense to re-examine the function(s) where the warning has come up, too. Where were you getting this warning?

@mzyx-hnu
Copy link
Author

mzyx-hnu commented Oct 8, 2023

However, it may also make sense to re-examine the function(s) where the warning has come up, too. Where were you getting this warning?

Sorry for the late reply. I'll post the log when this problem occurrs on AArch64(I don't have a x86 evironment to try on).

bash-5.2# make -j128 KERNELRELEASE=6.4.0-5.0.0.13.oe2309.aarch64 -C /lib/modules/6.4.0-5.0.0.13.oe2309.aarch64/build M=/var/lib/dkms/kmod-kvdo/8.2.1.2-3/build CC=clang V=1
make -f ./scripts/Makefile.build obj=/var/lib/dkms/kmod-kvdo/8.2.1.2-3/build need-builtin=1 need-modorder=1
make -f ./scripts/Makefile.build obj=/var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo \
need-builtin=1 \
need-modorder=1 \

# CC [M]  /var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/action-manager.o
  clang -Wp,-MMD,/var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/.action-manager.o.d -nostdinc -I./arch/arm64/include -I./arch/arm64/include/generated  -I./include -I./arch/arm64/include/uapi -I./arch/arm64/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -mlittle-endian -DCC_USING_PATCHABLE_FUNCTION_ENTRY -DKASAN_SHADOW_SCALE_SHIFT= -fmacro-prefix-map=./= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -funsigned-char -std=gnu11 --target=aarch64-linux-gnu -fintegrated-as -Werror=unknown-warning-option -Werror=ignored-optimization-argument -Werror=option-ignored -Werror=unused-command-line-argument -mgeneral-regs-only -DCONFIG_CC_HAS_K_CONSTRAINT=1 -Wno-psabi -fno-asynchronous-unwind-tables -fno-unwind-tables -mbranch-protection=pac-ret -Wa,-march=armv8.5-a -DARM64_ASM_ARCH='"armv8.5-a"' -DKASAN_SHADOW_SCALE_SHIFT= -fno-delete-null-pointer-checks -Wno-frame-address -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fstack-protector-strong -Wno-gnu -Wno-unused-but-set-variable -Wno-unused-const-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls -fpatchable-function-entry=4,2 -falign-functions=8 -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -Wimplicit-fallthrough -fno-strict-overflow -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -Wno-initializer-overrides -Wno-format -Wformat-extra-args -Wformat-invalid-specifier -Wformat-zero-length -Wnonnull -Wformat-insufficient-args -Wno-sign-compare -Wno-pointer-to-enum-cast -Wno-tautological-constant-out-of-range-compare -Wno-unaligned-access -g -gdwarf-4 -mstack-protector-guard=sysreg -mstack-protector-guard-reg=sp_el0 -mstack-protector-guard-offset=1816 -std=gnu99 -fno-builtin-memset -Werror -Wframe-larger-than=400 -DCURRENT_VERSION=\"8.2.1.2\" -I/var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo  -DMODULE  -DKBUILD_BASENAME='"action_manager"' -DKBUILD_MODNAME='"kvdo"' -D__KBUILD_MODNAME=kmod_kvdo -c -o /var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/action-manager.o /var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/action-manager.c
# CC [M]  /var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/volume-index006.o
  clang -Wp,-MMD,/var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/.volume-index006.o.d -nostdinc -I./arch/arm64/include -I./arch/arm64/include/generated  -I./include -I./arch/arm64/include/uapi -I./arch/arm64/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -mlittle-endian -DCC_USING_PATCHABLE_FUNCTION_ENTRY -DKASAN_SHADOW_SCALE_SHIFT= -fmacro-prefix-map=./= -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Werror=return-type -Wno-format-security -funsigned-char -std=gnu11 --target=aarch64-linux-gnu -fintegrated-as -Werror=unknown-warning-option -Werror=ignored-optimization-argument -Werror=option-ignored -Werror=unused-command-line-argument -mgeneral-regs-only -DCONFIG_CC_HAS_K_CONSTRAINT=1 -Wno-psabi -fno-asynchronous-unwind-tables -fno-unwind-tables -mbranch-protection=pac-ret -Wa,-march=armv8.5-a -DARM64_ASM_ARCH='"armv8.5-a"' -DKASAN_SHADOW_SCALE_SHIFT= -fno-delete-null-pointer-checks -Wno-frame-address -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fstack-protector-strong -Wno-gnu -Wno-unused-but-set-variable -Wno-unused-const-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls -fpatchable-function-entry=4,2 -falign-functions=8 -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wcast-function-type -Wimplicit-fallthrough -fno-strict-overflow -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -Wno-initializer-overrides -Wno-format -Wformat-extra-args -Wformat-invalid-specifier -Wformat-zero-length -Wnonnull -Wformat-insufficient-args -Wno-sign-compare -Wno-pointer-to-enum-cast -Wno-tautological-constant-out-of-range-compare -Wno-unaligned-access -g -gdwarf-4 -mstack-protector-guard=sysreg -mstack-protector-guard-reg=sp_el0 -mstack-protector-guard-offset=1816 -std=gnu99 -fno-builtin-memset -Werror -Wframe-larger-than=400 -DCURRENT_VERSION=\"8.2.1.2\" -I/var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo  -DMODULE  -DKBUILD_BASENAME='"volume_index006"' -DKBUILD_MODNAME='"kvdo"' -D__KBUILD_MODNAME=kmod_kvdo -c -o /var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/volume-index006.o /var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/volume-index006.c
/var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/volume-index006.c:639:5: error: stack frame size (432) exceeds limit (400) in 'compute_volume_index_save_bytes006' [-Werror,-Wframe-larger-than]
int compute_volume_index_save_bytes006(const struct configuration *config,
    ^
/var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/volume-index006.c:667:5: error: stack frame size (464) exceeds limit (400) in 'make_volume_index006' [-Werror,-Wframe-larger-than]
int make_volume_index006(const struct configuration *config,
    ^
2 errors generated.
make[2]: *** [scripts/Makefile.build:252: /var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/volume-index006.o] Error 1
make[2]: *** Waiting for unfinished jobs....
/var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/action-manager.c:114:5: error: stack frame size (416) exceeds limit (400) in 'vdo_make_action_manager' [-Werror,-Wframe-larger-than]
int vdo_make_action_manager(zone_count_t zones,
    ^
1 error generated.
make[2]: *** [scripts/Makefile.build:252: /var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo/action-manager.o] Error 1
make[1]: *** [scripts/Makefile.build:494: /var/lib/dkms/kmod-kvdo/8.2.1.2-3/build/vdo] Error 2
make: *** [Makefile:2026: /var/lib/dkms/kmod-kvdo/8.2.1.2-3/build] Error 2
# volume-index006.c:639
int compute_volume_index_save_bytes006(const struct configuration *config,
                                       size_t *num_bytes)
{
        size_t hook_bytes, non_hook_bytes;
        struct split_config split;
        int result = split_configuration006(config, &split);

        if (result != UDS_SUCCESS) {
                return result;
        }
        result = compute_volume_index_save_bytes005(&split.hook_config,
                                                    &hook_bytes);
        if (result != UDS_SUCCESS) {
                return result;
        }
        result = compute_volume_index_save_bytes005(&split.non_hook_config,
                                                    &non_hook_bytes);
        if (result != UDS_SUCCESS) {
                return result;
        }
        /*
         * Saving a volume index 006 needs a header plus the hook index plus
         * the non-hook index
         */
        *num_bytes = sizeof(struct vi006_data) + hook_bytes + non_hook_bytes;
        return UDS_SUCCESS;
}

I guess the stack space in these places is mainly occupied by struct split_config.

Hmm... actually it looks like our only clang builds use kernel snapshots, our latest working snapshot is from a week ago. Also, we’re using the clang compiler distributed with Fedora 37, so if that’s out of date, a newer one might do its stack allocations or its frame size calculations differently.

Another question of confusion is that I found the clang version of Fedora 37 is 15.0.7(the same version as I'm using). Looks like I need to find out what's the difference between the version I'm using and the version on fedora 37 that causes the stack size to be larger at compile time.

@mzyx-hnu
Copy link
Author

mzyx-hnu commented Oct 8, 2023

As for the difference between how gcc and llvm stack sizes are calculated, I wrote a small example to analyze (On AArch64, with clang15&gcc12).

struct CC {
  int cc[22];
  int *pc;
};

struct BB {
  int bb[10];
  float ff[12];
  char c[8];
  double d[2];
  struct CC cc;
};

struct AA {
  int a[10];
  int *b;
  char c[10];
  float f[5];
  struct BB b1;
  struct BB b2;
  struct CC c1;
  struct CC c2;
};

int main()
{
  struct AA ba;
  char * ca = alloca(100);
  return ba.c1.cc[0];
}

In the preceding code, the stack frame size calculated by gcc is 688 (struct) + 8 (char*) + 8 (16-byte alignment) = 704. The size of clang calculation is 8 (stack pointer) + 16 (fixed 16 word offset) + 688 + 100 (alloca space) + 8 + 4 (return value) + 8 (16-byte alignment) = 832. The range of clang calculation is more comprehensive. So the result is larger and closer to the actual stack size used.

@raeburn
Copy link
Member

raeburn commented Oct 10, 2023

Yes, split_config looks like the worst culprit there, though if clang is expanding the compute_volume_index_save_bytes005 calls inline, its automatic storage will contribute as well. Packing those structures better could help, but I think raising the limit in the clang case is pretty reasonable. Maybe by a small fixed amount to account for fixed differences in the calculations? I don’t think alloca would be used in our code anywhere so that can be ignored...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants