Skip to content
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

prog/alloc: align address allocation for aligned[addr] #1894

Merged
merged 2 commits into from
Jul 14, 2020

Conversation

albertlinde
Copy link
Collaborator

AlignAttr was only used to set padding and not to align to the
given argument. Existing descriptions with [align[X]] don't have
an issue as they align to small blocks and default align is to 64.
This commits adds support for [align[X]] for an X larger than 64.


Before sending a pull request, please review Contribution Guidelines:
https://github.com/google/syzkaller/blob/master/docs/contributing.md


@codecov
Copy link

codecov bot commented Jul 2, 2020

Codecov Report

Merging #1894 into master will increase coverage by 0.0%.
The diff coverage is 86.8%.

Impacted Files Coverage Δ
prog/target.go 62.3% <0.0%> (+0.8%) ⬆️
prog/alloc.go 77.0% <50.0%> (-1.6%) ⬇️
prog/types.go 71.9% <66.7%> (-0.1%) ⬇️
pkg/compiler/gen.go 93.9% <100.0%> (+0.4%) ⬆️
pkg/compiler/types.go 96.4% <100.0%> (+0.3%) ⬆️
prog/encoding.go 84.9% <100.0%> (ø)
prog/rand.go 92.3% <100.0%> (ø)
tools/syz-trace2syz/proggen/proggen.go 62.2% <100.0%> (ø)

Copy link
Collaborator

@melver melver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

prog/alloc.go Outdated Show resolved Hide resolved
prog/alloc.go Outdated Show resolved Hide resolved
prog/alloc.go Outdated Show resolved Hide resolved
prog/alloc.go Outdated Show resolved Hide resolved
prog/target.go Outdated Show resolved Hide resolved
prog/target.go Outdated Show resolved Hide resolved
prog/encoding.go Outdated Show resolved Hide resolved
prog/types.go Outdated Show resolved Hide resolved
prog/types.go Outdated Show resolved Hide resolved
prog/types.go Outdated Show resolved Hide resolved
prog/types.go Outdated Show resolved Hide resolved
prog/prog.go Outdated Show resolved Hide resolved
prog/any.go Outdated Show resolved Hide resolved
pkg/compiler/types.go Outdated Show resolved Hide resolved
Type.Alignment() can be used to obtain byte alignment for
correctly allocating aligned memory for the Type.
Calls to alloc didn't respect the alignment attribute. Now
Type.Alignment() is used to ensure each type is correctly
aligned. Existing descriptions with [align[X]] don't have an
issue as they align to small blocks and default align is to
64 bytes. This commits adds support for [align[X]] for an X
larger than 64.
@albertlinde
Copy link
Collaborator Author

@dvyukov I've tried to remove AlignAttr from StructType to use TypeAlign instead, but there are places where it is used -- and where using TypeAlign gives different semantics.
TypeAlign must depend on the alignment of inner elements, but for example here: any.go and prog.go, padding depends on the AlignAttr attribute.

It seems to me that we need two variables. If we use one and it is set to for example 4, we can't know if it is due inner elements or the attribute.

@dvyukov
Copy link
Collaborator

dvyukov commented Jul 14, 2020

@dvyukov I've tried to remove AlignAttr from StructType to use TypeAlign instead, but there are places where it is used -- and where using TypeAlign gives different semantics.

OK, let's keep.
But at least we don't need to add PackedAttr, right? We did not use it before.

TypeAlign must depend on the alignment of inner elements,

If there is alignment attribute, then it overrides any filed alignment:
https://github.com/google/syzkaller/blob/master/pkg/compiler/gen.go#L454

but I think there still may be some differences for variable-sized structs:
with explicit align attribute we will add padding after the struct dynamically (your prog.go link)

but for example here: any.go and prog.go, padding depends on the AlignAttr attribute.

It seems to me that we need two variables. If we use one and it is set to for example 4, we can't know if it is due inner elements or the attribute.

@dvyukov dvyukov marked this pull request as ready for review July 14, 2020 10:19
@dvyukov
Copy link
Collaborator

dvyukov commented Jul 14, 2020

Nice! That's a useful capability that nicely fits into the code base. Thanks.

@dvyukov dvyukov merged commit 6f45802 into google:master Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants