Re: [PATCH] libgccjit: Add support for setting the alignment [PR104293]

[Antoni Boucher <bouanto@zoho.com>]

[-- Attachment #1: Type: text/plain, Size: 2472 bytes --]

It seems there are some errors in the struct constructor code.
The attached patch seems to fix the issue.

The code from this bug report doesn't use the new API for alignment
(gcc_jit_lvalue_set_alignment); it uses the former one
(gcc_jit_type_get_aligned).
Perhaps it would be worth testing with the new API.

On Sat, 2022-04-16 at 22:26 +0200, Marc Nieper-Wißkirchen wrote:
> I haven't checked the cause of the bug reported here [1]. It may
> either be in the struct constructor code or in the new code for
> alignments, so this email goes to both Antoni and Petter. Speaking of
> struct constructors, I have found another bug [2]
> 
> Thanks,
> 
> Marc
> 
> --
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105296
> [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105286
> 
> Am Mi., 13. Apr. 2022 um 13:47 Uhr schrieb Bernhard Reutner-Fischer
> via Jit <jit@gcc.gnu.org>:
> > 
> > On 12 April 2022 23:51:34 CEST, David Malcolm via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > > On Sat, 2022-04-09 at 13:50 -0400, Antoni Boucher wrote:
> > > > Here's the updated patch.
> > > > 
> > 
> > > I've pushed the patch to trunk for GCC 12 as r12-8120-
> > > g6e5ad1cc24a315.
> > > 
> > > https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=6e5ad1cc24a315d07f24c95d76c269cafe2a8182
> > 
> > 
> > > > > > +   in C, but in bits instead of bytes.
> > > > > 
> > > > > If we're doing it in bytes, this will need updating, of
> > > > > course.
> > > > > 
> > > > > Maybe rename the int param from "alignment" to "bytes" to
> > > > > make this
> > > > > clearer.
> > > > > 
> > > > > Probably should be unsigned as well.
> > 
> > > > > > +void
> > > > > > +gcc_jit_lvalue_set_alignment (gcc_jit_lvalue *lvalue,
> > > > > > +                             int alignment)
> > > > > > +{
> > > > > > +  RETURN_IF_FAIL (lvalue, NULL, NULL, "NULL lvalue");
> > > > > 
> > > > > Should the alignment be unsigned?  What if the user passes in
> > > > > negative?
> > > > > 
> > > > > Does it have to be a power of two?  If so, ideally we should
> > > > > reject
> > > > > non-power-of-two here.
> > 
> > The wrapper was changed to unsigned bytes.
> > set_alignment still takes int bytes AFAICS?
> > 
> > Now I think there are or may be machine dependent max alignment,
> > aren't there? If there are, they are enforced later in the pipeline
> > I assume?
> > 
> > thanks,


[-- Attachment #2: 0001-Fix-usage-of-aligned-type-in-struct-constructor.patch --]
[-- Type: text/x-patch, Size: 1769 bytes --]

From 0bbab5376cb88564b33a6bbdeaf3187f802c24c8 Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Sat, 16 Apr 2022 22:25:41 -0400
Subject: [PATCH] Fix usage of aligned type in struct constructor

---
 gcc/jit/libgccjit.cc | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index ce54cc82a7a..a417e40d728 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -1414,16 +1414,16 @@ gcc_jit_context_new_struct_constructor (gcc_jit_context *ctxt,
   JIT_LOG_FUNC (ctxt->get_logger ());
   RETURN_NULL_IF_FAIL (type, ctxt, loc, "NULL type");
 
-  RETURN_NULL_IF_FAIL_PRINTF1 (type->is_struct (),
+  struct_* struct_type = type->is_struct ();
+  RETURN_NULL_IF_FAIL_PRINTF1 (struct_type,
 			       ctxt, loc,
 			       "constructor type is not a struct: %s",
 			       type->get_debug_string ());
 
-  compound_type *ct = reinterpret_cast<compound_type *>(type);
-  gcc::jit::recording::fields *fields_struct = ct->get_fields ();
+  gcc::jit::recording::fields *fields_struct = struct_type->get_fields ();
   size_t n_fields = fields_struct->length ();
 
-  RETURN_NULL_IF_FAIL_PRINTF1 (ct->has_known_size (),
+  RETURN_NULL_IF_FAIL_PRINTF1 (struct_type->has_known_size (),
 			       ctxt, loc,
 			       "struct can't be opaque: %s",
 			       type->get_debug_string ());
@@ -1472,7 +1472,7 @@ gcc_jit_context_new_struct_constructor (gcc_jit_context *ctxt,
 
 	  RETURN_NULL_IF_FAIL_PRINTF3 (
 	    f->get_container () ==
-	    static_cast<gcc::jit::recording::type*>(type),
+	    static_cast<gcc::jit::recording::type*>(struct_type),
 	    ctxt, loc,
 	    "field object at index %zu (%s), was not used when creating "
 	    "the %s",
-- 
2.26.2.7.g19db9cfb68.dirty