Skip to content

Fixed coverity issue in acl_device.cpp #225

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
Dec 8, 2022

Conversation

haoxian2
Copy link
Contributor

@haoxian2 haoxian2 commented Dec 7, 2022

Fixed coverity issue in acl_device.cpp: Type: Argument cannot be negative (NEGATIVE_RETURNS)

acl_get_default_device_global_memory can return -1 if no device memory is found. By static_casting it to size_t, it makes the -1 into something irrationally big. Therefore, after returning from the function, I added a check to see if gmem_id is negative. If it is negative, then I return 0. I assumed that no device private memory means that by default the sizes of global memory, of max constant buffer, and of max device memory allocation are all 0.

@haoxian2 haoxian2 force-pushed the coverity-acl-device branch 2 times, most recently from f611a03 to 22fa499 Compare December 7, 2022 19:45
@pcolberg pcolberg added the bug Something isn't working label Dec 7, 2022
@pcolberg pcolberg added this to the 2023.1 milestone Dec 7, 2022
@haoxian2 haoxian2 requested review from pcolberg and zibaiwan December 7, 2022 21:11
@haoxian2
Copy link
Contributor Author

haoxian2 commented Dec 8, 2022

I get this weird git warning whenever I pan over to Files changed tab:

Check warning on line 1 in src/acl_device.cpp

GitHub Actions
/ build

Coverage

82.3% (-1.1%)

I'm not sure if I'm supposed to do anything about it.

@haoxian2 haoxian2 force-pushed the coverity-acl-device branch from 22fa499 to 02f642c Compare December 8, 2022 15:25
`acl_get_default_device_global_memory` can return `-1` if no device memory is found. By static_casting it to `size_t`, it makes the -1 into something irrationally big. Therefore, after returning from the function, I added a check to see if `gmem_id` is negative. If it is negative, then I return 0. I assumed that no device private memory means that by default the sizes of global memory, of max constant buffer, and of max device memory allocation are all `0`.
@haoxian2 haoxian2 force-pushed the coverity-acl-device branch from 02f642c to 8633df2 Compare December 8, 2022 15:28
@haoxian2 haoxian2 requested a review from pcolberg December 8, 2022 15:30
@zibaiwan
Copy link
Contributor

zibaiwan commented Dec 8, 2022

I get this weird git warning whenever I pan over to Files changed tab:

Check warning on line 1 in src/acl_device.cpp

GitHub Actions
/ build

Coverage

82.3% (-1.1%)

I'm not sure if I'm supposed to do anything about it.

Thanks Hao. I don't see anything when I clicked on the Files changed tab. Maybe it's telling you that the "code coverage" workflow is running?

@haoxian2
Copy link
Contributor Author

haoxian2 commented Dec 8, 2022

This is the log that I get from coverage status in build log:

Warning: 82.3% (-1.1%)
File Name                         	Coverage (%) 	Delta Coverage      
---------                         	------------ 	--------------      
./src/acl_auto_configure.cpp      	92.82        	0.0                 
./src/acl_bsp_io.cpp              	0.0          	0.0                 
./src/acl_command.cpp             	82.5         	0.0                 
./src/acl_command_queue.cpp       	84.82        	0.0                 
./src/acl_context.cpp             	88.21        	0.0                 
./src/acl_device.cpp              	82.33        	-1.0799999999999983 
./src/acl_device_binary.cpp       	76.8         	0.0                 
./src/acl_device_op.cpp           	88.55        	0.0                 
./src/acl_device_program_info.cpp 	44.58        	0.0                 
./src/acl_event.cpp               	91.19        	0.0                 
./src/acl_globals.cpp             	75.19        	0.0                 
./src/acl_hal.cpp                 	79.71        	0.0                 
./src/acl_hal_mmd.cpp             	12.74        	0.0                 
./src/acl_hostch.cpp              	0.88         	0.0                 
./src/acl_icd_dispatch.cpp        	0.0          	0.0                 
./src/acl_kernel.cpp              	75.62        	0.0                 
./src/acl_kernel_if.cpp           	0.0          	0.0                 
./src/acl_mem.cpp                 	74.59       	0.0                 
./src/acl_offline_hal.cpp         	26.17        	0.0                 
./src/acl_platform.cpp            	82.0         	0.0                 
./src/acl_pll.cpp                 	0.0          	0.0                 
./src/acl_printf.cpp              	45.66        	0.0                 
./src/acl_profiler.cpp            	71.88        	0.0                 
./src/acl_program.cpp             	76.89        	0.0                 
./src/acl_sampler.cpp             	1.25         	0.0                 
./src/acl_shared_aligned_ptr.cpp  	100.0        	0.0                 
./src/acl_support.cpp             	66.44        	0.0                 
./src/acl_svm.cpp                 	70.47        	0.0                 
./src/acl_thread.cpp              	70.21        	0.0                 
./src/acl_usm.cpp                 	74.71        	0.0                 
./src/check_copy_overlap.c        	81.48        	0.0                 
./src/acl_device.cpp: Coverage decreased slightly for less than 20%

I'm not sure what all of this means...

@pcolberg
Copy link
Contributor

pcolberg commented Dec 8, 2022

@haoxian2 This is the code coverage workflow seeing the new branch for gmem_id < 0 which is not unit tested. A dummy device would need to be prepared which makes acl_get_default_device_global_memory() return an error, which is out of scope for this change and more suited for a future initiative that improves coverage.

@pcolberg pcolberg merged commit df770c0 into intel:main Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants