-
Notifications
You must be signed in to change notification settings - Fork 41
Fix for issue 97. Aligns all var_args on the strongest alignment constraint #98
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
base: master
Are you sure you want to change the base?
Changes from all commits
ca4e910
0fad263
c60f5df
6e9c25b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,13 @@ def __init__(self, context): | |
| BasicType.LONGDOUBLE: (ir.f64, 8), # TODO: is this correct? | ||
| } | ||
| self._constant_evaluator = LinkTimeExpressionEvaluator(self) | ||
| # Get strongest alignment constraint to align args in vararg block | ||
| # TODO should also take "double" alignment if defined for the target | ||
| # architecture for now, no easy way to get it | ||
| self.va_alignment = max( | ||
| self.context.arch_info.get_alignment("long"), | ||
| self.context.arch_info.get_alignment("ptr"), | ||
| ) | ||
|
|
||
| def get_label_block(self, name): | ||
| """ Get the ir block for a given label, and create it if necessary """ | ||
|
|
@@ -1451,39 +1458,31 @@ def gen_fill_varargs(self, var_args): | |
| """ | ||
| if var_args: | ||
| # Allocate a memory slab: | ||
| size = 0 | ||
| alignment = 1 | ||
| for va in var_args: | ||
| va_size, va_alignment = self.data_layout(va.typ) | ||
| # If not aligned, make it happen: | ||
| size += required_padding(size, va_alignment) | ||
| size += va_size | ||
| alignment = max(alignment, va_alignment) | ||
| # each argument is stored in a slot, aligned on the strongest alignment | ||
| size = self.va_alignment * len(var_args) | ||
| alignment = self.va_alignment | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about structs larger than the va_alignment size, is this allowed? Or can only basic types be passed via vararg way?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no type restrictions in the C standard, although I have never seen composite types passed to vararg functions. In function calls; we can copy their value starting at one slot and occupying several slots. After the copy we realign the vararg pointer to the beginning of next slot. |
||
|
|
||
| vararg_alloc = self.emit(ir.Alloc("varargs", size, alignment)) | ||
| vararg_ptr = self.emit(ir.AddressOf(vararg_alloc, "vaptr")) | ||
| vararg_ptr2 = vararg_ptr | ||
|
|
||
| offset = 0 | ||
| arg_count = len(var_args) | ||
| for argument in var_args: | ||
| value = self.gen_expr(argument, rvalue=True) | ||
| va_size, va_alignment = self.data_layout(argument.typ) | ||
|
|
||
| # handle alignment: | ||
| padding = required_padding(offset, va_alignment) | ||
| if padding > 0: | ||
| offset += padding | ||
| vararg_ptr2 = self.builder.emit_add( | ||
| vararg_ptr2, padding, ir.ptr | ||
| ) | ||
|
|
||
| # Store value: | ||
| self.emit(ir.Store(value, vararg_ptr2)) | ||
|
|
||
| # Increase pointer: | ||
| offset += va_size | ||
| vararg_ptr2 = self.builder.emit_add( | ||
| vararg_ptr2, va_size, ir.ptr | ||
| ) | ||
| # Update remaining number of arguments | ||
| arg_count -= 1 | ||
|
|
||
| # Increase pointer to next slot if necessary: | ||
| if arg_count: | ||
| offset += alignment | ||
| vararg_ptr2 = self.builder.emit_add( | ||
| vararg_ptr2, alignment, ir.ptr | ||
| ) | ||
| else: | ||
| # Emit a null pointer when no arguments given: | ||
| vararg_ptr = self.emit(ir.Const(0, "varargs", ir.ptr)) | ||
|
|
@@ -1554,13 +1553,14 @@ def gen_va_start(self, expr: expressions.BuiltInVaStart): | |
|
|
||
| def gen_va_arg(self, expr: expressions.BuiltInVaArg): | ||
| """ Generate code for a va_arg operation """ | ||
| # TODO: how to deal with proper alignment? | ||
| # all arguments are all in similar slots | ||
| valist_ptrptr = self.gen_expr(expr.arg_pointer, rvalue=False) | ||
| va_ptr = self.emit(ir.Load(valist_ptrptr, "va_ptr", ir.ptr)) | ||
| ir_typ = self.get_ir_type(expr.typ) | ||
| # Load the variable argument: | ||
| ir_typ = self.get_ir_type(expr.typ) | ||
| value = self.emit(ir.Load(va_ptr, "va_arg", ir_typ)) | ||
| size = self.emit(ir.Const(self.sizeof(expr.typ), "size", ir.ptr)) | ||
| # Increment pointer to next slot | ||
| size = self.emit(ir.Const(self.va_alignment, "size", ir.ptr)) | ||
| va_ptr = self.emit(ir.add(va_ptr, size, "incptr", ir.ptr)) | ||
| self.emit(ir.Store(va_ptr, valist_ptrptr)) | ||
| return value | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename this to
defines_globalThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's more consistent.