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

templates: fix mismatched type size in simple #140

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

colorglass
Copy link
Contributor

@colorglass colorglass commented Jan 29, 2024

Fix this #139. Using macro to select the build-in function in different type size.

For more background and discussion see also seL4/util_libs#176

@axel-h
Copy link
Member

axel-h commented Feb 1, 2024

Based on seL4/util_libs#176 (comment) it seems
the minimal fix is simply using __builtin_clzl() then.

@colorglass
Copy link
Contributor Author

Yes, we can just use CLZL() macro since it can handle the word type correctly.

Fix a mismatched type size error in component.simple.c:485, which may
causes data overflow when CONFIG_WORD_SIZE is 64 in the 64-bit
 machine. Using defined CLZL() macro to correctly handle the specified
CONFIG_WORD_SIZE.

Signed-off-by: colorglass <55863235+colorglass@users.noreply.github.com>
Copy link
Member

@axel-h axel-h left a comment

Choose a reason for hiding this comment

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

Looks good to me now.

You may want to raise an issue separately for supporting the LLP64 data model for 64-bit userland. Seem not many people have tries doing this using the "Windows" compiler.

@colorglass
Copy link
Contributor Author

Sorry for my bothering.
This PR seems to be still linked to the closed seL4/util_libs#176, and the check got failed.
Is there any thing I can do to fix it? This is my first PR experience in github, and forgive my ignorance to this.

@lsf37
Copy link
Member

lsf37 commented Feb 2, 2024

The error in the build seems to be

/github/workspace/projects/util_libs/libutils/include/utils/builtin.h:30:36: error: expected ‘)’ before ‘sizeof’
     30 | compile_time_assert(long_is_32bits, sizeof(long) == 4)
        |                                    ^~~~~~~
        |                                    )

The syntax looks correct to me, but the compiler seems to disagree :-). Might need to track down the compile_time_assert definition that is used here.

Edit: just saw that is this is not even part of your change. Including utils/builtin.h already seem to trigger this, which is a bit unfortunate. Maybe there is some interaction with the inclusion of assert.h before. @axel-h do you know anything about that?

@colorglass
Copy link
Contributor Author

/github/workspace/projects/util_libs/libutils/include/utils/builtin.h:30:36: error: expected ‘)’ before ‘sizeof’
     30 | compile_time_assert(long_is_32bits, sizeof(long) == 4)
        |                                    ^~~~~~~
        |                                    )

😢 Its my shame that I forgot to add the corresponded header file in that PR commit. And it was closed without the fix.

@axel-h
Copy link
Member

axel-h commented Feb 2, 2024

The "Test with" was still in the PR description when this was pushed. Removed now and re-running, so CI will just use this PR and not the other one also.

@axel-h axel-h added the bug label Feb 2, 2024
@lsf37
Copy link
Member

lsf37 commented Feb 2, 2024

Good catch, thanks @axel-h. The link checker is unhappy with graphviz for some reason today, but that is unrelated. Good to merge from my side.

@lsf37
Copy link
Member

lsf37 commented Feb 2, 2024

Thanks for the contribution, @colorglass

@lsf37 lsf37 merged commit 11836b1 into seL4:master Feb 2, 2024
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants