On Wed, Oct 16, 2019 at 11:55 AM Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote: > > 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. Good to know, but I want less duct tape, not more. > > > > > > > > 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. That's fair. I've included your suggestions in the commit message of 02/03 of a series I just sent but forgot to in reply to this thread: https://lkml.org/lkml/2019/10/16/1700 Also, I do appreciate the suggestions and understand the value of brainstorming. -- Thanks, ~Nick Desaulniers _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
