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

alignchecker: fully parse structures #24365

Merged
merged 1 commit into from Mar 30, 2023

Conversation

aspsk
Copy link
Contributor

@aspsk aspsk commented Mar 14, 2023

The alignchecker package takes BTF descriptions of structures and creates a map with offsets. For example,

struct x {
    u64 a;
    u32 b;
    u8 c;
    u8 d;
};

will be converted to

{a: 0, b: 8, c: 12, d: 13}

These names are later used in Go struct definitions, so that we can check if they are properly aligned, e.g.:

type X struct {
    A uint64 `align:"a"`
    B uint32 `align:"b"`
    C uint8 `align:"c"`
    D uint8 `align:"d"`
   }

If there are anonymous unions present, then their offsets will be stored as $union0, $union1, etc., e.g.,

struct x {
    union { u32 a; u32 b; };
    union { u32 c; u32 d; };
    u32 f;
};

will be converted to

{$union0: 0, $union1: 4, f: 8}

However, there's no way to refer to inner fields of unions, or to fields of structures. Fix this by parsing BTFs recursively. After this change, e.g., the following structure

struct x {
    u32 a;
    struct {
        union {
            u32 a;
            u32 b;
        };
        u32 c;
    };
    u32 d;
};

will be converted to

{
    a: 0,
    $struct0: 4,
    $struct0.$union0: 4,
    $struct0.$union0.a: 4,
    $struct0.$union0.b: 4,
    $struct0.c: 8,
    d: 12
}

Also fix a minor bug: previous code didn't check if an offset exists or not (it was set to 0 by default). Patch two occurrences in code which were referencing non-existing offset (it happened that the corresponding offsets were indeed 0).

@aspsk aspsk added the release-note/misc This PR makes changes that have no direct user impact. label Mar 14, 2023
@aspsk aspsk requested review from a team as code owners March 14, 2023 15:52
@aspsk aspsk requested a review from jibi March 14, 2023 15:52
@aspsk aspsk force-pushed the aspsk/pr/improve-alignchecker branch from 5d72feb to 444831d Compare March 14, 2023 16:05
The alignchecker package takes BTF descriptions of structures and creates a map
with offsets. For example,

    struct x {
        u64 a;
        u32 b;
        u8 c;
        u8 d;
    };

will be converted to

    {a: 0, b: 8, c: 12, d: 13}

These names are later used in Go struct definitions, so that we can check if they are properly aligned, e.g.:

    type X struct {
        A uint64 `align:"a"`
        B uint32 `align:"b"`
        C uint8 `align:"c"`
        D uint8 `align:"d"`
       }

If there are anonymous unions present, then their offsets will be stored as
$union0, $union1, etc., e.g.,

    struct x {
        union { u32 a; u32 b; };
        union { u32 c; u32 d; };
        u32 f;
    };

will be converted to

    {$union0: 0, $union1: 4, f: 8}

However, there's no way to refer to inner fields of unions, or to fields of
structures. Fix this by parsing BTFs recursively. After this change, e.g., the
following structure

    struct x {
        u32 a;
        struct {
            union {
                u32 a;
                u32 b;
            };
            u32 c;
        };
        u32 d;
    };

will be converted to

    {
        a: 0,
        $struct0: 4,
        $struct0.$union0: 4,
        $struct0.$union0.a: 4,
        $struct0.$union0.b: 4,
        $struct0.c: 8,
        d: 12
    }

Also fix a minor bug: previous code didn't check if an offset exists or not (it
was set to 0 by default). Patch two occurrences in code which were referencing
non-existing offset (it happened that the corresponding offsets were indeed 0).

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
@aspsk
Copy link
Contributor Author

aspsk commented Mar 14, 2023

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: migrate-svc restart count values do not match

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing Tests with secondary NodePort device

Failure Output

FAIL: Can not connect to service "tftp://[fd05::11]:30786/hello" from outside cluster (8/10)

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

@ciliumbot
Copy link

Build finished.

@aspsk
Copy link
Contributor Author

aspsk commented Mar 15, 2023

The net-next flake looks like #23309

The 4.19 flake looks like #23845

@aspsk
Copy link
Contributor Author

aspsk commented Mar 15, 2023

/test-1.16-4.19

@aspsk
Copy link
Contributor Author

aspsk commented Mar 15, 2023

/test-1.26-net-next

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: migrate-svc restart count values do not match

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

@aspsk
Copy link
Contributor Author

aspsk commented Mar 21, 2023

/test-1.16-4.19

@aspsk
Copy link
Contributor Author

aspsk commented Mar 30, 2023

The test-1.16-4.19 is unrelated, so marking as ready to merge

@aspsk aspsk added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 30, 2023
@julianwiedmann
Copy link
Member

julianwiedmann commented Mar 30, 2023

The test-1.16-4.19 is unrelated, so marking as ready to merge

For this sort of situation, please point at the actual CI issue that tracks the unrelated failure. The Jenkins data has already expired ...

@julianwiedmann julianwiedmann merged commit 938d88a into cilium:master Mar 30, 2023
@aspsk aspsk deleted the aspsk/pr/improve-alignchecker branch March 30, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants