From owner-freebsd-ppc@freebsd.org Sat Feb 20 08:37:15 2016 Return-Path: Delivered-To: freebsd-ppc@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2791FAAFA0C; Sat, 20 Feb 2016 08:37:15 +0000 (UTC) (envelope-from rdivacky@vlakno.cz) Received: from vlakno.cz (mail.vlakno.cz [91.217.96.224]) by mx1.freebsd.org (Postfix) with ESMTP id C8C501239; Sat, 20 Feb 2016 08:37:14 +0000 (UTC) (envelope-from rdivacky@vlakno.cz) Received: by vlakno.cz (Postfix, from userid 1002) id 7F43D1E22A88; Sat, 20 Feb 2016 09:34:50 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=vlakno.cz; s=mail; t=1455957290; bh=QeAgPO+laCwfHsBItkk1oUsDqVnOsBvhqYSTSemvQIs=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=AsopzhD23XVbeR2mzblbmOQzpUBzJoALjl4OlQfsV7mab5hJCmlno/HfgZQiSXB6W HQSWHzmcstggIDpn2ypLo5EB0adDX/kikO00SkE+y0uXSWyTESERukVNMS+rWUVje8 mbw9kK+vM3WV/cIBhG1BUg15e1N798C2nZNqI3tQ= Date: Sat, 20 Feb 2016 09:34:50 +0100 From: Roman Divacky To: Mark Millard Cc: FreeBSD Toolchain , FreeBSD PowerPC ML Subject: Re: I've submitted 207175 for a clang 3.8.0 va_list handling problem for powerpc [fpr use also tested] Message-ID: <20160220083450.GA55777@vlakno.cz> References: <20160215191100.GA17387@vlakno.cz> <3A260EC5-E69A-4980-8F74-C04395F4E5F4@dsl-only.net> <20160215201800.GA20796@vlakno.cz> <74577A87-3006-43A9-9EAB-F51D946B6245@dsl-only.net> <28FF474D-2109-4605-8B2B-C5374CBCCF42@dsl-only.net> <8EB46124-3335-4643-8C64-16DA56D481F5@dsl-only.net> <8C40A5D7-C0B8-4142-89D4-228017C446CE@dsl-only.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8C40A5D7-C0B8-4142-89D4-228017C446CE@dsl-only.net> User-Agent: Mutt/1.5.24 (2015-08-30) X-BeenThere: freebsd-ppc@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Porting FreeBSD to the PowerPC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 20 Feb 2016 08:37:15 -0000 Fwiw, I've just committed the patch to clang in r261422. You might want to keep using a local modification or ask dim@ to import that patch to our copy of 3.8. Thanks for your diagnosis and testing! Roman On Thu, Feb 18, 2016 at 02:29:29PM -0800, Mark Millard wrote: > On 2016-Feb-17, at 9:23 PM, Mark Millard wrote: > > > > My fpr related notes/worries about the fix were wrong. > > > > I finally got some time to look at this again and I see that I somehow missed the following code when I looked before: > > > > // The calling convention either uses 1-2 GPRs or 1 FPR. > > Address NumRegsAddr = Address::invalid(); > > if (isInt || IsSoftFloatABI) { > > NumRegsAddr = Builder.CreateStructGEP(VAList, 0, CharUnits::Zero(), "gpr"); > > } else { > > NumRegsAddr = Builder.CreateStructGEP(VAList, 1, CharUnits::One(), "fpr"); > > } > > > > So the > > > > Builder.CreateStore(Builder.getInt8(8), NumRegsAddr); > > > > in Case 2 is tracking gpr vs. fpr usage contexts as it should. Also: > > > > llvm::Value *NumRegs = Builder.CreateLoad(NumRegsAddr, "numUsedRegs"); > > > > // "Align" the register count when TY is i64. > > if (isI64 || (isF64 && IsSoftFloatABI)) { > > NumRegs = Builder.CreateAdd(NumRegs, Builder.getInt8(1)); > > NumRegs = Builder.CreateAnd(NumRegs, Builder.getInt8((uint8_t) ~1U)); > > } > > > > llvm::Value *CC = > > Builder.CreateICmpULT(NumRegs, Builder.getInt8(8), "cond"); > > > > is using the same bounds check figure (8) for gpr and fpr. > > > > Apparently that common bound is one reason that the clang numbering is not the same as the ABI document's numbering: clang's numbering allows using the same figure for both contexts. (Given the prior alignment for isI64 (or isF64 with IsSoftFloatABI).) > > > > Sorry for the prior noise about fpr. > > > > It is still true that DOUBLE_OR_FLOAT is untested so far. > > > > === > > Mark Millard > > markmi at dsl-only.net > > I finally got some time to apply to some basic testing involving double as well (for involving fpr use). . . > > No problems with exceptions. Looking at the memory contents at various stages in gdb looks good. va_list's gpr, fpr, overflow_arg_area changes as its va_args use progresses look good. Values extracted by va_args use looks good. Both default and -O2. > > The added > > > Builder.CreateStore(Builder.getInt8(8), NumRegsAddr); > > > passes my checks. I've not observed any problems from buildworld materials, unlike when that line is missing. > > [Note: I run with the signal delivery modified to have a "red zone" to deal with other aspects of clang 3.8.0 code generation that are not ABI compliant for when the stack pointer is moved. Having a "red zone" is still operationally correct for an ABI compliant code generation, it just temporarily wastes more bytes. Also: the kernel was built with gcc 4.2.1 but world was built with clang 3.8.0.] > > > === > Mark Millard > markmi at dsl-only.net > > . . . [bad fpr related material omitted] . . . > > On 2016-Feb-16, at 2:45 AM, Mark Millard wrote: > > I used: > > > Index: /usr/src/contrib/llvm/tools/clang/lib/CodeGen/TargetInfo.cpp > > =================================================================== > > --- /usr/src/contrib/llvm/tools/clang/lib/CodeGen/TargetInfo.cpp (revision 295601) > > +++ /usr/src/contrib/llvm/tools/clang/lib/CodeGen/TargetInfo.cpp (working copy) > > @@ -3569,6 +3569,8 @@ > > { > > CGF.EmitBlock(UsingOverflow); > > > > + Builder.CreateStore(Builder.getInt8(8), NumRegsAddr); > > + > > // Everything in the overflow area is rounded up to a size of at least 4. > > CharUnits OverflowAreaAlign = CharUnits::fromQuantity(4); > > as my test change. (Get evidence of operation before potential cleanup of the duplicated 8's.) > > After a full buildworld/installworld based on the updated compiler. . . > > My simple example of the problem no longer fails. > > "ls -l -n /" now works. > > "svnlite update -r295601 /usr/src" now works. > > So whatever you want to do for the details of any submitted code, the basics of the change do avoid the SEGVs and allow these programs to work. > > > === > Mark Millard > markmi at dsl-only.net > > On 2016-Feb-15, at 4:27 PM, Mark Millard wrote: > > On 2016-Feb-15, at 1:20 PM, Mark Millard wrote: > > > > On 2016-Feb-15, at 12:18 PM, Roman Divacky wrote: > >> > >> On Mon, Feb 15, 2016 at 12:17:50PM -0800, Mark Millard wrote: > >>> On 2016-Feb-15, at 11:11 AM, Roman Divacky wrote: > >>>> > >>>> Mark, I believe you're right. What do you think about this patch? > >>>> > >>>> Index: tools/clang/lib/CodeGen/TargetInfo.cpp > >>>> =================================================================== > >>>> --- tools/clang/lib/CodeGen/TargetInfo.cpp (revision 260852) > >>>> +++ tools/clang/lib/CodeGen/TargetInfo.cpp (working copy) > >>>> @@ -3599,6 +3599,8 @@ > >>>> { > >>>> CGF.EmitBlock(UsingOverflow); > >>>> > >>>> + Builder.CreateStore(Builder.getInt8(11), NumRegsAddr); > >>>> + > >>>> // Everything in the overflow area is rounded up to a size of at least 4. > >>>> CharUnits OverflowAreaAlign = CharUnits::fromQuantity(4); > >>>> > >>>> > >>>> Can you test it? > >>> > >>> It may be later today before I can start the the test process. > >>> > >>> While your change is not wrong as presented, it does seem to be based on the ABI document's numbering with the range 3 <= gr <12, where 3 <= gr < 11 cover r3-r10 use and gr=11 implies overflow stack area use. (gr being the ABI documents name.) > >>> > >>> The clang code generation that I saw while analyzing the problem and the clang source that you had me look at did not use that numbering. Instead it seems to be based on 0 <= gpr < 9, where 0 <= gpr < 8 cover r3-r10 use and gpr=8 implies overflow stack area use. (gpr being what gdb showed me as I remember.) In other words: clang counts the number of "parameter registers" already in use as it goes along instead of tracking register numbers that have been used. > >>> > >>> So assigning any value that appears to be positive and >= 8 should work, such as: > >>> > >>> Builder.CreateStore(Builder.getInt8(8), NumRegsAddr); > >> > >> Can you check what number gcc uses? We want to be interoperable with gcc. > >> > >> Anyway, thanks for testing! > >> > >> Roman > > > > I'll do that check of gcc 4.2.1 code generation before starting the test later today. > > > > But if the clang numbering is different in gcc 4.2.1 then far more than just adding a > > > >> Builder.CreateStore(Builder.getInt8(?), NumRegsAddr) > > > > > > for some "?" would need to be involved in the changes in order to reach compatibility. > > > > > > I'll note that for clang 3.8.0 the actual comparison instruction generated is of the form > > > >> cmplwi r?,7 > > > > > > for some r?, such as r5 or r4, and the conditional branch generated is a bgt instruction. > > > > === > > Mark Millard > > markmi at dsl-only.net > > gcc 4.2.1 generates comparison instructions for va_arg of the form: > > cmplwi cr7,r0,8 > > and the conditional branch generated is a "bge cr7, . . ." instruction. > > So the same number range is in use by both compilers: They are compatible for the bounds checks for reg vs. overflow for how they count, equality inclusion/exclusion matching up with the specific number (8 vs. 7) used to make things the same overall. > > Other aspects of the code generation distinctions would take me time to analyze. It will be a while before I will be looking at other points. > > > === > Mark Millard > markmi at dsl-only.net > > > > > > > _______________________________________________ > freebsd-toolchain@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-toolchain > To unsubscribe, send any mail to "freebsd-toolchain-unsubscribe@freebsd.org"