Skip to content

Modified strcture data element from fixed size to flexible array. #8997

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

Merged
merged 1 commit into from
Jun 10, 2025

Conversation

ManojTakasi
Copy link
Collaborator

Problem solved by the commit

This PR is cherry-picked from #8669 and #8746

https://jira.xilinx.com/browse/CR-1214907 We are getting UBSAN: array-index-out-of-bounds call trace while compiling xocl xclmgmt drivers in ubuntu 24.04

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
Reference: https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays

How problem was solved, alternative solutions (if any) and why they were rejected
Added macros to pick flexible arrays when comipiling kernel code.

Risks (if any) associated the changes in the commit
Modifications are in common files and will affect all platforms.

What has been tested and how, request additional testing if necessary

Documentation impact (if any)

Copy link
Collaborator

@stsoe stsoe left a comment

Choose a reason for hiding this comment

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

I am not comfortable with ert.h changes from data[1] -> data[]. While I completely agree that we should have used data[] from inception of this code, changing it now has ripple effects, e.g. sizeof(ert_start_kernel_cmd) changes, and who knows what XRT is relying doing and relying on?

I am all for data[] for all new structs added, but leave existing ones alone.

@stsoe
Copy link
Collaborator

stsoe commented Jun 3, 2025

I am not comfortable with ert.h changes from data[1] -> data[]. While I completely agree that we should have used data[] from inception of this code, changing it now has ripple effects, e.g. sizeof(ert_start_kernel_cmd) changes, and who knows what XRT is relying doing and relying on?

I am all for data[] for all new structs added, but leave existing ones alone.

I take that back given that this is a cherry pick from code that was merged to master long ago. Please ignore my comment above. Also, user space is unchanged, so whatever it does with sizeof is unaffected (good). It would be good if we had a project to consolidate all code on data[], which is exactly the right to use.

@chvamshi-xilinx
Copy link
Collaborator

@ManojTakasi , As this is a big change. Can you please make sure edge tests also works with this change. You can try couple of basic tests.

@ManojTakasi
Copy link
Collaborator Author

Hi @chvamshi-xilinx, I tested the vadd and simple_graph applications after applying these changes, and both tests working as expected.

@chvamshi-xilinx chvamshi-xilinx merged commit d8cf77a into Xilinx:2024.2 Jun 10, 2025
62 of 69 checks passed
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