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.

Sign up for GitHub

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

Jump to bottom

Re-enabling narrowing errors #1144

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Re-enabling narrowing errors #1144

wants to merge 4 commits into from

Conversation

ellnor01
Copy link
Contributor

@ellnor01 ellnor01 commented Feb 3, 2021 •
edited
Loading

Fixes narrowing conversion build errors in test_common

Removing disable of narrowing errors in main CMakeLists.txt
and moving it down to specific test_conformance suite's
CMakeLists.txt where there are many more build errors revealed
from this fix.

Fixes a few simple issues under test_conformance in the process.

Contributes #787

Signed-off-by: Ellen Norris-Thompson ellen.norris-thompson@arm.com

@ellnor01 ellnor01 force-pushed the narrowing_conversion_1 branch 5 times, most recently from e41f811 to f46c622 Compare February 10, 2021 10:05
@ellnor01 ellnor01 mentioned this pull request Feb 12, 2021
Open
@ellnor01
Copy link
Contributor Author

Any comments on here?

This was referenced May 20, 2021
Open
Open
Open
Open
@kpet
Copy link
Contributor

kpet commented Jul 3, 2024

@ahesham-arm This is failing the CI. It may require a rebase and/or fixes.

@ahesham-arm ahesham-arm force-pushed the narrowing_conversion_1 branch 3 times, most recently from 08da74d to 2b418b0 Compare July 7, 2024 14:15
@ahesham-arm
Copy link
Contributor

@kpet Rebased and all CI checks are now passing.

bashbaug
bashbaug reviewed Jul 9, 2024
View reviewed changes
test_conformance/extensions/cl_khr_command_buffer/command_buffer_printf.cpp Outdated
@@ -274,7 +274,7 @@ struct CommandBufferPrintfTest : public BasicCommandBufferTest
&pattern[0], 0, nullptr, nullptr);
test_error(error, "clEnqueueWriteBuffer failed");

cl_int offset[] = { 0, pattern.size() - 1 };
size_t offset[] = { 0, pattern.size() - 1 };
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please check that this change is correct? I'm nervous that going from a 32-bit type (cl_int) to a possibly 64-bit type (size_t) may require some additional changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

offset is used in 3 locations in this function, two are trivial loop comparisons against another size_t, so those are fine. The other, main one, is a call to clEnqueueWriteBuffer which expects a void* and the size in bytes. The call is already using sizeof so I don't see why this would fail regardless of the data type and/or architecture.

test_conformance/atomics/test_atomics.cpp Outdated
Comment on lines 1295 to 1296
TestFns set = { (int)0xffffffff,
(long long)0xffffffffffffffffLL,
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to cast these to the types used in the structure, which are guaranteed to have a fixed size, rather than int and long long that could be different sizes for different compilers.

Suggested change
TestFns set = { (int)0xffffffff,
(long long)0xffffffffffffffffLL,
TestFns set = { (cl_int)0xffffffff,
(cl_long)0xffffffffffffffffLL,

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

lakshmih
lakshmih reviewed Jul 23, 2024
View reviewed changes
test_conformance/extensions/cl_khr_command_buffer/command_buffer_printf.cpp
@@ -274,7 +274,7 @@ struct CommandBufferPrintfTest : public BasicCommandBufferTest
&pattern[0], 0, nullptr, nullptr);
test_error(error, "clEnqueueWriteBuffer failed");

cl_int offset[] = { 0, pattern.size() - 1 };
size_t offset[] = { 0, pattern.size() - 1 };
error = clEnqueueWriteBuffer(queue, off_mem, CL_TRUE, 0, sizeof(offset),
offset, 0, nullptr, nullptr);
test_error(error, "clEnqueueWriteBuffer failed");
Copy link
Contributor

Choose a reason for hiding this comment

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

off_mem is created for sizeof(cl_int) size so I think the enqueuewritebuffer to it using sizeof(size_t) can cause issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is now using cl_uint for the type of the array and an assert is added to ensure the casting is safe.

lakshmih
lakshmih reviewed Jul 23, 2024
View reviewed changes
test_conformance/extensions/cl_khr_command_buffer/negative_command_buffer_create.cpp
@@ -115,7 +115,8 @@ struct CreateCommandBufferRepeatedProperties : public BasicCommandBufferTest
TEST_FAIL);

cl_command_buffer_properties_khr invalid_properties[3] = {
CL_COMMAND_BUFFER_FLAGS_KHR, CL_INVALID_PROPERTY, 0
CL_COMMAND_BUFFER_FLAGS_KHR,
(cl_command_buffer_properties_khr)CL_INVALID_PROPERTY, 0
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure CL_INVALID_PROPERTY can be used in a property list like this. The spec only specifies its usage as a return value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have used -1 instead.

lakshmih
lakshmih requested changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@lakshmih lakshmih left a comment

Choose a reason for hiding this comment

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

Left some comments

@ellnor01 @ahesham-arm
Re-enabling narrowing errors
65412cc 
Fixes narrowing conversion build errors in test_common

Removing disable of narrowing errors in main CMakeLists.txt
and moving it down to test_conformance CMakeLists.txt where
there are many more build errors revealed from this fix

Contributes KhronosGroup#787

Signed-off-by: Ellen Norris-Thompson <ellen.norris-thompson@arm.com>
@ellnor01 @ahesham-arm
Enable narrowing errors for more directories
fdc4090 
Moves -Wno-narrowing down to only run on suites with
many narrowing errors.

Includes some quick fixes for one-off errors this caused.

Contributes KhronosGroup#787

Signed-off-by: Ellen Norris-Thompson <ellen.norris-thompson@arm.com>
@ellnor01 @ahesham-arm
Make Version function to_uint() for consistency
5afa2a4 
Includes fix for MacOS build error

Contributes KhronosGroup#787

Signed-off-by: Ellen Norris-Thompson <ellen.norris-thompson@arm.com>
@ellnor01 @ahesham-arm
Fix narrowing errors in compiler and spirv_new
a27cdc3 
Contributes KhronosGroup#787

Signed-off-by: Ellen Norris-Thompson <ellen.norris-thompson@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bashbaug bashbaug bashbaug left review comments

@jlewis-austin jlewis-austin jlewis-austin left review comments

@ahesham-arm ahesham-arm ahesham-arm left review comments

@lakshmih lakshmih lakshmih requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
focused review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ellnor01 @kpet @ahesham-arm @lakshmih @bashbaug @jlewis-austin