On Tue, Oct 15, 2019 at 06:51:26PM -0700, Nick Desaulniers wrote: > On Tue, Oct 15, 2019 at 1:26 PM Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote: > > > > On Tue, Oct 15, 2019 at 11:05:56AM -0700, Nick Desaulniers wrote: > > > Hmmm...I would have liked to remove it outright, as it is an ABI > > > mismatch that is likely to result in instability and non-fun-to-debug > > > runtime issues in the future. I suspect my patch does work for GCC > > > 7.1+. The question is: Do we want to either: > > > 1. mark AMDGPU broken for GCC < 7.1, or > > > 2. continue supporting it via stack alignment mismatch? > > > > > > 2 is brittle, and may break at any point in the future, but if it's > > > working for someone it does make me feel bad to outright disable it. > > > What I'd image 2 looks like is (psuedo code in a Makefile): > > > > > > if CC_IS_GCC && GCC_VERSION < 7.1: > > > set stack alignment to 16B and hope for the best > > > > > > So my diff would be amended to keep the stack alignment flags, but > > > only to support GCC < 7.1. And that assumes my change compiles with > > > GCC 7.1+. (Looks like it does for me locally with GCC 8.3, but I would > > > feel even more confident if someone with hardware to test on and GCC > > > 7.1+ could boot test). > > > -- > > > Thanks, > > > ~Nick Desaulniers > > > > If we do keep it, would adding -mstackrealign make it more robust? > > That's simple and will only add the alignment to functions that require > > 16-byte alignment (at least on gcc). > > I think there's also `-mincoming-stack-boundary=`. > https://github.com/ClangBuiltLinux/linux/issues/735#issuecomment-540038017 Yes, but -mstackrealign looks like it's supported by clang as well. > > > > > Alternative is to use > > __attribute__((force_align_arg_pointer)) on functions that might be > > called from 8-byte-aligned code. > > Which is hard to automate and easy to forget. Likely a large diff to fix today. Right, this is a no-go, esp to just fix old compilers. > > > > > It looks like -mstackrealign should work from gcc 5.3 onwards. > > The kernel would generally like to support GCC 4.9+. > > There's plenty of different ways to keep layering on duct tape and > bailing wire to support differing ABIs, but that's just adding > technical debt that will have to be repaid one day. That's why the > cleanest solution IMO is mark the driver broken for old toolchains, > and use a code-base-consistent stack alignment. Bending over > backwards to support old toolchains means accepting stack alignment > mismatches, which is in the "unspecified behavior" ring of the > "undefined behavior" Venn diagram. I have the same opinion on relying > on explicitly undefined behavior. > > I'll send patches for fixing up Clang, but please consider my strong > advice to generally avoid stack alignment mismatches, regardless of > compiler. > -- > Thanks, > ~Nick Desaulniers What I suggested was in reference to your proposal for dropping the -mpreferred-stack-boundary=4 for modern compilers, but keeping it for <7.1. -mstackrealign would at least let 5.3 onwards be less likely to break (and it doesn't error before then, I think it just doesn't actually do anything, so no worse than now at least). Simply dropping support for <7.1 would be cleanest, yes, but it sounds like people don't want to go that far. _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
