From 9a2b9e2feb0aa9598f334883d15a6a9800d53623 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Wed, 14 Aug 2024 10:05:56 -0700 Subject: [PATCH 01/27] keep this --- src/printf.cpp | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index d4766619..2b6e3ac0 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -166,15 +166,22 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Firstly print the part of the format string up to the first '%' size_t next_part = format_string.find_first_of('%'); + if (next_part > format_string.size()) { + return; + } printf_out << format_string.substr(0, next_part); // Decompose the remaining format string into individual strings with // one format specifier each, handle each one individually size_t arg_idx = 0; while (next_part < format_string.size() - 1) { + auto tmp = printf_out.str().c_str(); // Get the part of the format string before the next format specifier size_t part_start = next_part; size_t part_end = format_string.find_first_of('%', part_start + 1); + if (part_end >= format_string.size()) { + part_end = format_string.size() - 1; + } auto part_fmt = format_string.substr(part_start, part_end - part_start); // Handle special cases @@ -209,8 +216,16 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Special case for %s if (get_fmt_conversion(part_fmt) == 's') { uint32_t string_id = read_buff(data); - printf_out << print_part( - part_fmt, descs.at(string_id).format_string.c_str(), size); + if (string_id >= descs.size()) { + printf_out << ""; + } else { + auto curr = print_part( + part_fmt, descs.at(string_id).format_string.c_str(), + size); + printf_out << print_part( + part_fmt, descs.at(string_id).format_string.c_str(), + size); + } } else { printf_out << print_part(part_fmt, data, size); } @@ -238,7 +253,7 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, next_part = part_end; arg_idx++; } - + auto tmp = printf_out.str().c_str(); printf("%s", printf_out.str().c_str()); } From 8046dc2b5a0aa7731734b3d548730aa821c5c872 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Wed, 14 Aug 2024 15:57:20 -0700 Subject: [PATCH 02/27] rework how process_printf handles strings --- src/printf.cpp | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index 2b6e3ac0..1d314cc2 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -175,19 +175,41 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // one format specifier each, handle each one individually size_t arg_idx = 0; while (next_part < format_string.size() - 1) { - auto tmp = printf_out.str().c_str(); // Get the part of the format string before the next format specifier size_t part_start = next_part; size_t part_end = format_string.find_first_of('%', part_start + 1); - if (part_end >= format_string.size()) { - part_end = format_string.size() - 1; + + // Handle case where there's no second '%' + if (part_end == std::string::npos || part_end >= format_string.size()) { + part_end = + format_string.size(); // Set part_end to the end of the string } auto part_fmt = format_string.substr(part_start, part_end - part_start); // Handle special cases - if (part_end == part_start + 1) { - printf_out << "%"; - next_part = part_end + 1; + if (part_fmt == "%") { + // printf requires two % i.e. %% to display % iff they are not at + // the start of the string. All starting % shoulld be collapsed into + // one. + if (printf_out.str() != part_fmt) { + printf_out << part_fmt; + // check for two consecutive % since otherwise the way we are + // moving the next_part var this will be skipped. + if (format_string[part_start] == '%' && + printf_out.str() != part_fmt) { + printf_out << part_fmt; + } + } + next_part = part_end; + // Skip the next % as well since we have already added it. + if (format_string[part_start] == '%') { + next_part++; + } + continue; + // Single char + } else if (part_fmt.length() == 1) { + printf_out << part_fmt; + next_part = part_end; continue; } else if (part_end == std::string::npos && arg_idx >= descs.at(printf_id).arg_sizes.size()) { @@ -219,9 +241,6 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, if (string_id >= descs.size()) { printf_out << ""; } else { - auto curr = print_part( - part_fmt, descs.at(string_id).format_string.c_str(), - size); printf_out << print_part( part_fmt, descs.at(string_id).format_string.c_str(), size); @@ -253,7 +272,6 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, next_part = part_end; arg_idx++; } - auto tmp = printf_out.str().c_str(); printf("%s", printf_out.str().c_str()); } From 4a33381085a389ff3c0e90fbb82916886fdd6a63 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Wed, 14 Aug 2024 16:04:28 -0700 Subject: [PATCH 03/27] add missing space --- src/printf.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/printf.cpp b/src/printf.cpp index 1d314cc2..cee4e0bf 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -272,6 +272,7 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, next_part = part_end; arg_idx++; } + printf("%s", printf_out.str().c_str()); } From 45f2ae47c0840f11e0f7cc79a77b7e4428741182 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Tue, 27 Aug 2024 15:53:53 -0700 Subject: [PATCH 04/27] fix cases with no format --- src/printf.cpp | 194 ++++++++++++++++++++++++++----------------------- 1 file changed, 102 insertions(+), 92 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index cee4e0bf..9e113eb6 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -167,110 +167,120 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Firstly print the part of the format string up to the first '%' size_t next_part = format_string.find_first_of('%'); if (next_part > format_string.size()) { - return; - } - printf_out << format_string.substr(0, next_part); - - // Decompose the remaining format string into individual strings with - // one format specifier each, handle each one individually - size_t arg_idx = 0; - while (next_part < format_string.size() - 1) { - // Get the part of the format string before the next format specifier - size_t part_start = next_part; - size_t part_end = format_string.find_first_of('%', part_start + 1); - - // Handle case where there's no second '%' - if (part_end == std::string::npos || part_end >= format_string.size()) { - part_end = - format_string.size(); // Set part_end to the end of the string + // if there is no format string followed by a string to print just print + // the whole string itself. + if (descs.size() == 1) { + printf_out << format_string; } - auto part_fmt = format_string.substr(part_start, part_end - part_start); - - // Handle special cases - if (part_fmt == "%") { - // printf requires two % i.e. %% to display % iff they are not at - // the start of the string. All starting % shoulld be collapsed into - // one. - if (printf_out.str() != part_fmt) { - printf_out << part_fmt; - // check for two consecutive % since otherwise the way we are - // moving the next_part var this will be skipped. - if (format_string[part_start] == '%' && - printf_out.str() != part_fmt) { + } else { + printf_out << format_string.substr(0, next_part); + + // Decompose the remaining format string into individual strings with + // one format specifier each, handle each one individually + size_t arg_idx = 0; + while (next_part < format_string.size() - 1) { + // Get the part of the format string before the next format + // specifier + size_t part_start = next_part; + size_t part_end = format_string.find_first_of('%', part_start + 1); + + // Handle case where there's no second '%' + if (part_end == std::string::npos || + part_end >= format_string.size()) { + part_end = format_string + .size(); // Set part_end to the end of the string + } + auto part_fmt = + format_string.substr(part_start, part_end - part_start); + + // Handle special cases + if (part_fmt == "%") { + // printf requires two % i.e. %% to display % iff they are not + // at the start of the string. All starting % shoulld be + // collapsed into one. + if (printf_out.str() != part_fmt) { printf_out << part_fmt; + // check for two consecutive % since otherwise the way we + // are moving the next_part var this will be skipped. + if (format_string[part_start] == '%' && + printf_out.str() != part_fmt) { + printf_out << part_fmt; + } } + next_part = part_end; + // Skip the next % as well since we have already added it. + if (format_string[part_start] == '%') { + next_part++; + } + continue; + // Single char + } else if (part_fmt.length() == 1) { + printf_out << part_fmt; + next_part = part_end; + continue; + } else if (part_end == std::string::npos && + arg_idx >= descs.at(printf_id).arg_sizes.size()) { + // If there are no remaining arguments, the rest of the format + // should be printed verbatim + printf_out << part_fmt; + break; } - next_part = part_end; - // Skip the next % as well since we have already added it. - if (format_string[part_start] == '%') { - next_part++; - } - continue; - // Single char - } else if (part_fmt.length() == 1) { - printf_out << part_fmt; - next_part = part_end; - continue; - } else if (part_end == std::string::npos && - arg_idx >= descs.at(printf_id).arg_sizes.size()) { - // If there are no remaining arguments, the rest of the format - // should be printed verbatim - printf_out << part_fmt; - break; - } - // The size of the argument that this format part will consume - auto& size = descs.at(printf_id).arg_sizes[arg_idx]; + // The size of the argument that this format part will consume + auto& size = descs.at(printf_id).arg_sizes[arg_idx]; - if (data + size > data_end) { - data += size; - return; - } + if (data + size > data_end) { + data += size; + return; + } - // Check to see if we have a vector format specifier - int vec_len = 0; - int el_size = 0; - std::string remaining_str; - part_fmt = get_vector_fmt(part_fmt, vec_len, el_size, remaining_str); - - // Scalar argument - if (vec_len < 2) { - // Special case for %s - if (get_fmt_conversion(part_fmt) == 's') { - uint32_t string_id = read_buff(data); - if (string_id >= descs.size()) { - printf_out << ""; + // Check to see if we have a vector format specifier + int vec_len = 0; + int el_size = 0; + std::string remaining_str; + part_fmt = + get_vector_fmt(part_fmt, vec_len, el_size, remaining_str); + + // Scalar argument + if (vec_len < 2) { + // Special case for %s + if (get_fmt_conversion(part_fmt) == 's') { + uint32_t string_id = read_buff(data); + if (string_id >= descs.size()) { + printf_out << ""; + } else { + printf_out << print_part( + part_fmt, descs.at(string_id).format_string.c_str(), + size); + } } else { - printf_out << print_part( - part_fmt, descs.at(string_id).format_string.c_str(), - size); + printf_out << print_part(part_fmt, data, size); } + data += size; } else { - printf_out << print_part(part_fmt, data, size); - } - data += size; - } else { - // Vector argument - if (el_size == 0) { - // 'ele_size == 0' means that no length modifier has been used. - // According to the spec, this is an undefined behavior. Let's - // use the size coming from clspv and the vec_len to figure out - // the element size then. - el_size = size / vec_len; - } - auto* data_start = data; - for (int i = 0; i < vec_len - 1; i++) { - printf_out << print_part(part_fmt, data, size / vec_len) << ","; - data += el_size; + // Vector argument + if (el_size == 0) { + // 'ele_size == 0' means that no length modifier has been + // used. According to the spec, this is an undefined + // behavior. Let's use the size coming from clspv and the + // vec_len to figure out the element size then. + el_size = size / vec_len; + } + auto* data_start = data; + for (int i = 0; i < vec_len - 1; i++) { + printf_out << print_part(part_fmt, data, size / vec_len) + << ","; + data += el_size; + } + printf_out << print_part(part_fmt, data, size / vec_len) + << remaining_str; + data = data_start + size; } - printf_out << print_part(part_fmt, data, size / vec_len) - << remaining_str; - data = data_start + size; - } - // Move to the next format part and prepare to handle the next arg - next_part = part_end; - arg_idx++; + // Move to the next format part and prepare to handle the next arg + next_part = part_end; + arg_idx++; + } } printf("%s", printf_out.str().c_str()); From a7af5cb319435e1121530706d11b0ac7477994f7 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Date: Wed, 28 Aug 2024 09:08:08 -0700 Subject: [PATCH 05/27] use assert instead of if and return earlier. Co-authored-by: Romaric Jodin --- src/printf.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index 9e113eb6..531a376e 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -167,12 +167,12 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Firstly print the part of the format string up to the first '%' size_t next_part = format_string.find_first_of('%'); if (next_part > format_string.size()) { - // if there is no format string followed by a string to print just print - // the whole string itself. - if (descs.size() == 1) { - printf_out << format_string; - } - } else { + // if there is no format just print the whole string itself. + assert (descs.size() == 1); + printf_out << format_string; + printf("%s", printf_out.str().c_str()); + return; + } printf_out << format_string.substr(0, next_part); // Decompose the remaining format string into individual strings with From 8c606c7c3b4aa0d95b6653c500b328c9cb44d210 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Wed, 28 Aug 2024 10:50:10 -0700 Subject: [PATCH 06/27] move data to end instead of asserting --- src/printf.cpp | 193 ++++++++++++++++++++++++------------------------- 1 file changed, 93 insertions(+), 100 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index 531a376e..5e226ced 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -167,122 +167,115 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Firstly print the part of the format string up to the first '%' size_t next_part = format_string.find_first_of('%'); if (next_part > format_string.size()) { - // if there is no format just print the whole string itself. - assert (descs.size() == 1); printf_out << format_string; printf("%s", printf_out.str().c_str()); + data = data_end; return; } - printf_out << format_string.substr(0, next_part); - - // Decompose the remaining format string into individual strings with - // one format specifier each, handle each one individually - size_t arg_idx = 0; - while (next_part < format_string.size() - 1) { - // Get the part of the format string before the next format - // specifier - size_t part_start = next_part; - size_t part_end = format_string.find_first_of('%', part_start + 1); - - // Handle case where there's no second '%' - if (part_end == std::string::npos || - part_end >= format_string.size()) { - part_end = format_string - .size(); // Set part_end to the end of the string - } - auto part_fmt = - format_string.substr(part_start, part_end - part_start); - - // Handle special cases - if (part_fmt == "%") { - // printf requires two % i.e. %% to display % iff they are not - // at the start of the string. All starting % shoulld be - // collapsed into one. - if (printf_out.str() != part_fmt) { + printf_out << format_string.substr(0, next_part); + + // Decompose the remaining format string into individual strings with + // one format specifier each, handle each one individually + size_t arg_idx = 0; + while (next_part < format_string.size() - 1) { + // Get the part of the format string before the next format + // specifier + size_t part_start = next_part; + size_t part_end = format_string.find_first_of('%', part_start + 1); + + // Handle case where there's no second '%' + if (part_end == std::string::npos || part_end >= format_string.size()) { + part_end = + format_string.size(); // Set part_end to the end of the string + } + auto part_fmt = format_string.substr(part_start, part_end - part_start); + + // Handle special cases + if (part_fmt == "%") { + // printf requires two % i.e. %% to display % iff they are not + // at the start of the string. All starting % shoulld be + // collapsed into one. + if (printf_out.str() != part_fmt) { + printf_out << part_fmt; + // check for two consecutive % since otherwise the way we + // are moving the next_part var this will be skipped. + if (format_string[part_start] == '%' && + printf_out.str() != part_fmt) { printf_out << part_fmt; - // check for two consecutive % since otherwise the way we - // are moving the next_part var this will be skipped. - if (format_string[part_start] == '%' && - printf_out.str() != part_fmt) { - printf_out << part_fmt; - } } - next_part = part_end; - // Skip the next % as well since we have already added it. - if (format_string[part_start] == '%') { - next_part++; - } - continue; - // Single char - } else if (part_fmt.length() == 1) { - printf_out << part_fmt; - next_part = part_end; - continue; - } else if (part_end == std::string::npos && - arg_idx >= descs.at(printf_id).arg_sizes.size()) { - // If there are no remaining arguments, the rest of the format - // should be printed verbatim - printf_out << part_fmt; - break; } + next_part = part_end; + // Skip the next % as well since we have already added it. + if (format_string[part_start] == '%') { + next_part++; + } + continue; + // Single char + } else if (part_fmt.length() == 1) { + printf_out << part_fmt; + next_part = part_end; + continue; + } else if (part_end == std::string::npos && + arg_idx >= descs.at(printf_id).arg_sizes.size()) { + // If there are no remaining arguments, the rest of the format + // should be printed verbatim + printf_out << part_fmt; + break; + } - // The size of the argument that this format part will consume - auto& size = descs.at(printf_id).arg_sizes[arg_idx]; + // The size of the argument that this format part will consume + auto& size = descs.at(printf_id).arg_sizes[arg_idx]; - if (data + size > data_end) { - data += size; - return; - } + if (data + size > data_end) { + data += size; + return; + } - // Check to see if we have a vector format specifier - int vec_len = 0; - int el_size = 0; - std::string remaining_str; - part_fmt = - get_vector_fmt(part_fmt, vec_len, el_size, remaining_str); - - // Scalar argument - if (vec_len < 2) { - // Special case for %s - if (get_fmt_conversion(part_fmt) == 's') { - uint32_t string_id = read_buff(data); - if (string_id >= descs.size()) { - printf_out << ""; - } else { - printf_out << print_part( - part_fmt, descs.at(string_id).format_string.c_str(), - size); - } + // Check to see if we have a vector format specifier + int vec_len = 0; + int el_size = 0; + std::string remaining_str; + part_fmt = get_vector_fmt(part_fmt, vec_len, el_size, remaining_str); + + // Scalar argument + if (vec_len < 2) { + // Special case for %s + if (get_fmt_conversion(part_fmt) == 's') { + uint32_t string_id = read_buff(data); + if (string_id >= descs.size()) { + printf_out << ""; } else { - printf_out << print_part(part_fmt, data, size); + printf_out << print_part( + part_fmt, descs.at(string_id).format_string.c_str(), + size); } - data += size; } else { - // Vector argument - if (el_size == 0) { - // 'ele_size == 0' means that no length modifier has been - // used. According to the spec, this is an undefined - // behavior. Let's use the size coming from clspv and the - // vec_len to figure out the element size then. - el_size = size / vec_len; - } - auto* data_start = data; - for (int i = 0; i < vec_len - 1; i++) { - printf_out << print_part(part_fmt, data, size / vec_len) - << ","; - data += el_size; - } - printf_out << print_part(part_fmt, data, size / vec_len) - << remaining_str; - data = data_start + size; + printf_out << print_part(part_fmt, data, size); } - - // Move to the next format part and prepare to handle the next arg - next_part = part_end; - arg_idx++; + data += size; + } else { + // Vector argument + if (el_size == 0) { + // 'ele_size == 0' means that no length modifier has been + // used. According to the spec, this is an undefined + // behavior. Let's use the size coming from clspv and the + // vec_len to figure out the element size then. + el_size = size / vec_len; + } + auto* data_start = data; + for (int i = 0; i < vec_len - 1; i++) { + printf_out << print_part(part_fmt, data, size / vec_len) << ","; + data += el_size; + } + printf_out << print_part(part_fmt, data, size / vec_len) + << remaining_str; + data = data_start + size; } - } + // Move to the next format part and prepare to handle the next arg + next_part = part_end; + arg_idx++; + } printf("%s", printf_out.str().c_str()); } From 1cd33652fb43590d0810d5d39fd4d78a3c89b4c1 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Date: Thu, 29 Aug 2024 09:31:38 -0700 Subject: [PATCH 07/27] reinstate comment format Co-authored-by: Romaric Jodin --- src/printf.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index d4c22eb6..ad6c3718 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -257,10 +257,10 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, } else { // Vector argument if (el_size == 0) { - // 'ele_size == 0' means that no length modifier has been - // used. According to the spec, this is an undefined - // behavior. Let's use the size coming from clspv and the - // vec_len to figure out the element size then. + // 'ele_size == 0' means that no length modifier has been used. + // According to the spec, this is an undefined behavior. Let's + // use the size coming from clspv and the vec_len to figure out + // the element size then. el_size = size / vec_len; } auto* data_start = data; From ff36633f99be345b799a5775bea48bf31ad211f9 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Date: Thu, 29 Aug 2024 09:31:49 -0700 Subject: [PATCH 08/27] reinstate comment format Co-authored-by: Romaric Jodin --- src/printf.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index ad6c3718..9c5892e7 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -179,8 +179,7 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // one format specifier each, handle each one individually size_t arg_idx = 0; while (next_part < format_string.size() - 1) { - // Get the part of the format string before the next format - // specifier + // Get the part of the format string before the next format specifier size_t part_start = next_part; size_t part_end = format_string.find_first_of('%', part_start + 1); From 33e43e6f39d39179b4827ebb798e24dddbb529e6 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Mon, 9 Sep 2024 14:34:33 -0700 Subject: [PATCH 09/27] add unit test for original failure --- tests/api/printf.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/api/printf.cpp b/tests/api/printf.cpp index cbade14c..56bd5dd0 100644 --- a/tests/api/printf.cpp +++ b/tests/api/printf.cpp @@ -134,6 +134,30 @@ TEST_F(WithCommandQueue, SimplePrintf) { ASSERT_STREQ(printf_buffer, message); } +TEST_F(WithCommandQueue, SimplePrintfWithFormat) { + temp_folder_deletion temp; + stdoutFileName = getStdoutFileName(temp); + + int fd; + ASSERT_TRUE(getStdout(fd)); + + const char message[] = ""; + char source[512]; + sprintf(source, "kernel void test_printf() { printf(\"%%s\", \"\"); }"); + auto kernel = CreateKernel(source, "test_printf"); + + size_t gws = 1; + size_t lws = 1; + EnqueueNDRangeKernel(kernel, 1, nullptr, &gws, &lws, 0, nullptr, nullptr); + Finish(); + + releaseStdout(fd); + auto printf_buffer = getStdoutContent(); + ASSERT_NE(printf_buffer, nullptr); + + ASSERT_STREQ(printf_buffer, message); +} + TEST_F(WithCommandQueueNoSetUp, TooLongPrintf) { std::string buffer = ""; // each print takes 12 bytes (4 for the printf_id, and 2*4 for the 2 integer From ebec0c4a87a06040d3c3b59c657a42254634c7e4 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Mon, 9 Sep 2024 14:59:51 -0700 Subject: [PATCH 10/27] rework test logic --- tests/api/printf.cpp | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/tests/api/printf.cpp b/tests/api/printf.cpp index 5165e8a8..4c753b5f 100644 --- a/tests/api/printf.cpp +++ b/tests/api/printf.cpp @@ -33,12 +33,6 @@ TEST_F(WithCommandQueueAndPrintf, SimplePrintf) { } TEST_F(WithCommandQueue, SimplePrintfWithFormat) { - temp_folder_deletion temp; - stdoutFileName = getStdoutFileName(temp); - - int fd; - ASSERT_TRUE(getStdout(fd)); - const char message[] = ""; char source[512]; sprintf(source, "kernel void test_printf() { printf(\"%%s\", \"\"); }"); @@ -49,11 +43,7 @@ TEST_F(WithCommandQueue, SimplePrintfWithFormat) { EnqueueNDRangeKernel(kernel, 1, nullptr, &gws, &lws, 0, nullptr, nullptr); Finish(); - releaseStdout(fd); - auto printf_buffer = getStdoutContent(); - ASSERT_NE(printf_buffer, nullptr); - - ASSERT_STREQ(printf_buffer, message); + ASSERT_STREQ(m_printf_output.c_str(), message); } TEST_F(WithCommandQueueAndPrintf, TooLongPrintf) { From 467ffade6cc1758f7998143eba2ceb23b4afc7ec Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Mon, 9 Sep 2024 15:06:20 -0700 Subject: [PATCH 11/27] use correct class --- tests/api/printf.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/api/printf.cpp b/tests/api/printf.cpp index 4c753b5f..dc4fd2eb 100644 --- a/tests/api/printf.cpp +++ b/tests/api/printf.cpp @@ -32,7 +32,7 @@ TEST_F(WithCommandQueueAndPrintf, SimplePrintf) { ASSERT_STREQ(m_printf_output.c_str(), message); } -TEST_F(WithCommandQueue, SimplePrintfWithFormat) { +TEST_F(WithCommandQueueAndPrintf, SimplePrintfWithFormat) { const char message[] = ""; char source[512]; sprintf(source, "kernel void test_printf() { printf(\"%%s\", \"\"); }"); From cbd9ff3f8a20345421378629ca42e6e25d541754 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Mon, 9 Sep 2024 15:44:04 -0700 Subject: [PATCH 12/27] rework logic --- src/printf.cpp | 3 --- tests/api/printf.cpp | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index 9c5892e7..2bd6062b 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -168,10 +168,7 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Firstly print the part of the format string up to the first '%' size_t next_part = format_string.find_first_of('%'); if (next_part > format_string.size()) { - printf_out << format_string; - printf("%s", printf_out.str().c_str()); data = data_end; - return; } printf_out << format_string.substr(0, next_part); diff --git a/tests/api/printf.cpp b/tests/api/printf.cpp index dc4fd2eb..70d23985 100644 --- a/tests/api/printf.cpp +++ b/tests/api/printf.cpp @@ -32,7 +32,7 @@ TEST_F(WithCommandQueueAndPrintf, SimplePrintf) { ASSERT_STREQ(m_printf_output.c_str(), message); } -TEST_F(WithCommandQueueAndPrintf, SimplePrintfWithFormat) { +TEST_F(WithCommandQueueAndPrintf, SimpleFormatedPrintf) { const char message[] = ""; char source[512]; sprintf(source, "kernel void test_printf() { printf(\"%%s\", \"\"); }"); From d0a44b6327fe3e58310d7fb565c38e581527d8a7 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Wed, 11 Sep 2024 09:35:21 -0700 Subject: [PATCH 13/27] get rid of sprintf --- tests/api/printf.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/api/printf.cpp b/tests/api/printf.cpp index 70d23985..58127a05 100644 --- a/tests/api/printf.cpp +++ b/tests/api/printf.cpp @@ -33,9 +33,7 @@ TEST_F(WithCommandQueueAndPrintf, SimplePrintf) { } TEST_F(WithCommandQueueAndPrintf, SimpleFormatedPrintf) { - const char message[] = ""; - char source[512]; - sprintf(source, "kernel void test_printf() { printf(\"%%s\", \"\"); }"); + const char* source = "kernel void test_printf() { printf(\"%s\", \"\"); }"; auto kernel = CreateKernel(source, "test_printf"); size_t gws = 1; @@ -43,7 +41,7 @@ TEST_F(WithCommandQueueAndPrintf, SimpleFormatedPrintf) { EnqueueNDRangeKernel(kernel, 1, nullptr, &gws, &lws, 0, nullptr, nullptr); Finish(); - ASSERT_STREQ(m_printf_output.c_str(), message); + ASSERT_STREQ(m_printf_output.c_str(), ""); } TEST_F(WithCommandQueueAndPrintf, TooLongPrintf) { From 7cca2d89b5992837f174c52ac781d42ded0a3d26 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Mon, 23 Sep 2024 17:08:31 -0700 Subject: [PATCH 14/27] Fix issues with format_string --- src/printf.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index 2bd6062b..9f64f0f4 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -191,13 +191,16 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, if (part_fmt == "%") { // printf requires two % i.e. %% to display % iff they are not // at the start of the string. All starting % shoulld be - // collapsed into one. + // collapsed into one unless there are exactly two % side by side. + if (printf_out.str() != part_fmt) { printf_out << part_fmt; // check for two consecutive % since otherwise the way we // are moving the next_part var this will be skipped. if (format_string[part_start] == '%' && - printf_out.str() != part_fmt) { + (printf_out.str() != part_fmt || + std::count(format_string.begin(), format_string.end(), + '%') == 2)) { printf_out << part_fmt; } } @@ -242,11 +245,15 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, if (string_id >= descs.size()) { printf_out << ""; } else { + auto tmmmp = print_part( + part_fmt, descs.at(string_id).format_string.c_str(), + size); printf_out << print_part( part_fmt, descs.at(string_id).format_string.c_str(), size); } } else { + auto tmmmp = print_part(part_fmt, data, size); printf_out << print_part(part_fmt, data, size); } data += size; From f688a80de9cb4de99762be84965e8e929357e475 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Tue, 24 Sep 2024 19:51:27 -0700 Subject: [PATCH 15/27] handle quotes --- src/printf.cpp | 73 +++++++++++++++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 19 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index 9f64f0f4..5ad0af18 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -13,12 +13,19 @@ // limitations under the License. #include +#include #include "printf.hpp" // Extract the conversion specifier from a format string char get_fmt_conversion(std::string_view fmt) { auto conversionSpecPos = fmt.find_first_of("diouxXfFeEgGaAcsp"); + // Check if a valid conversion specifier was found + if (conversionSpecPos == std::string_view::npos) { + conversionSpecPos = -1; + return '\0'; // Return null character to indicate + // no specifier found + } return fmt.at(conversionSpecPos); } @@ -156,6 +163,16 @@ std::string print_part(const std::string& fmt, const char* data, size_t size) { return std::string(out.data()); } +char get_quote(const std::string& str) { + if (str.find('"') != std::string::npos) { + return '"'; + } + if (str.find('\'') != std::string::npos) { + return '\''; + } + return '\0'; +} + void process_printf(char*& data, const printf_descriptor_map_t& descs, char* data_end, cvk_printf_callback_t printf_cb, void* printf_userdata) { @@ -175,6 +192,14 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Decompose the remaining format string into individual strings with // one format specifier each, handle each one individually size_t arg_idx = 0; + std::stack quotes; + if (format_string.size() > 0) { + auto curr_quote = get_quote(format_string.substr( + 0, next_part)); // checking if curr selection is inside a " or '. + if (curr_quote) { + quotes.push(curr_quote); + } + } while (next_part < format_string.size() - 1) { // Get the part of the format string before the next format specifier size_t part_start = next_part; @@ -187,22 +212,27 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, } auto part_fmt = format_string.substr(part_start, part_end - part_start); + auto curr_quote = get_quote(format_string.substr(part_start, part_end)); + if (curr_quote) { + if (!quotes.empty() && quotes.top() == curr_quote) { + quotes.pop(); + } else { + quotes.push(curr_quote); + } + } // Handle special cases if (part_fmt == "%") { // printf requires two % i.e. %% to display % iff they are not // at the start of the string. All starting % shoulld be // collapsed into one unless there are exactly two % side by side. - - if (printf_out.str() != part_fmt) { + printf_out << part_fmt; + // check for two consecutive % since otherwise the way we + // are moving the next_part var this will be skipped. + if (part_start + 1 < format_string.length() && + format_string[part_start + 1] == '%' && quotes.empty() && + !(part_start + 2 < format_string.length() && + format_string[part_start + 2] == 's')) { printf_out << part_fmt; - // check for two consecutive % since otherwise the way we - // are moving the next_part var this will be skipped. - if (format_string[part_start] == '%' && - (printf_out.str() != part_fmt || - std::count(format_string.begin(), format_string.end(), - '%') == 2)) { - printf_out << part_fmt; - } } next_part = part_end; // Skip the next % as well since we have already added it. @@ -224,11 +254,20 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, } // The size of the argument that this format part will consume - auto& size = descs.at(printf_id).arg_sizes[arg_idx]; + size_t size = 0; - if (data + size > data_end) { + if (descs.count(printf_id) > 0 && + !descs.at(printf_id).arg_sizes.empty() && + arg_idx < descs.at(printf_id).arg_sizes.size()) { + size = descs.at(printf_id).arg_sizes[arg_idx]; + } + + // Handle invalid cases (size will be 0 here) + if (size == 0 || data + size > data_end) { data += size; - return; + if (data > data_end) { + return; + } } // Check to see if we have a vector format specifier @@ -245,15 +284,11 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, if (string_id >= descs.size()) { printf_out << ""; } else { - auto tmmmp = print_part( - part_fmt, descs.at(string_id).format_string.c_str(), - size); printf_out << print_part( part_fmt, descs.at(string_id).format_string.c_str(), size); } } else { - auto tmmmp = print_part(part_fmt, data, size); printf_out << print_part(part_fmt, data, size); } data += size; @@ -283,8 +318,8 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, auto output = printf_out.str(); if (printf_cb != nullptr) { - auto len = output.size(); - printf_cb(output.c_str(), len, data >= data_end, printf_userdata); + printf_cb(output.c_str(), output.size(), data >= data_end, + printf_userdata); } else { printf("%s", output.c_str()); } From 95578406b3c07590cb686fc9f038a1ef9d465a36 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Thu, 26 Sep 2024 16:00:59 -0700 Subject: [PATCH 16/27] fix vector of len>10 and %% cases --- src/printf.cpp | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index 5ad0af18..f268af7a 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -22,7 +22,6 @@ char get_fmt_conversion(std::string_view fmt) { auto conversionSpecPos = fmt.find_first_of("diouxXfFeEgGaAcsp"); // Check if a valid conversion specifier was found if (conversionSpecPos == std::string_view::npos) { - conversionSpecPos = -1; return '\0'; // Return null character to indicate // no specifier found } @@ -64,7 +63,7 @@ std::string get_vector_fmt(std::string fmt, int& vector_size, int& element_size, size_t vec_length_pos_start = ++pos; size_t vec_length_pos_end = - fmt.find_first_not_of("23468", vec_length_pos_start); + fmt.find_first_not_of("23468", vec_length_pos_start + 1); auto vec_length_str = fmt.substr(vec_length_pos_start, vec_length_pos_end - vec_length_pos_start); int vec_length = std::atoi(vec_length_str.c_str()); @@ -195,7 +194,7 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, std::stack quotes; if (format_string.size() > 0) { auto curr_quote = get_quote(format_string.substr( - 0, next_part)); // checking if curr selection is inside a " or '. + 0, next_part)); // checking if curr selection has a " or '. if (curr_quote) { quotes.push(curr_quote); } @@ -222,14 +221,18 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, } // Handle special cases if (part_fmt == "%") { - // printf requires two % i.e. %% to display % iff they are not - // at the start of the string. All starting % shoulld be - // collapsed into one unless there are exactly two % side by side. printf_out << part_fmt; // check for two consecutive % since otherwise the way we // are moving the next_part var this will be skipped. - if (part_start + 1 < format_string.length() && - format_string[part_start + 1] == '%' && quotes.empty() && + // printf requires two % i.e. %% to display % iff they are not + // at the start of the string. All starting %% shoulld be + // collapsed into one %. + // also check the the two consecutive % is not a case of %%s + // if the two %% are in quotes then we will collapse it into one % + auto curr_str = printf_out.str(); + if (curr_str.length() > 1 && quotes.empty() && + part_start + 1 < format_string.length() && + format_string[part_start + 1] == '%' && !(part_start + 2 < format_string.length() && format_string[part_start + 2] == 's')) { printf_out << part_fmt; @@ -317,9 +320,13 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, } auto output = printf_out.str(); + // special case for single % which needs to be represented by two %%. + if (output == "%") { + output = "%%"; + } if (printf_cb != nullptr) { - printf_cb(output.c_str(), output.size(), data >= data_end, - printf_userdata); + auto len = output.size(); + printf_cb(output.c_str(), len, data >= data_end, printf_userdata); } else { printf("%s", output.c_str()); } From 2380ff8ababdaff87e0a6b321dabd0f4ac7baeb7 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Tue, 1 Oct 2024 00:19:15 -0700 Subject: [PATCH 17/27] make less complex --- src/printf.cpp | 95 +++++++++++--------------------------------------- 1 file changed, 21 insertions(+), 74 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index f268af7a..1f449642 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -13,14 +13,12 @@ // limitations under the License. #include -#include #include "printf.hpp" // Extract the conversion specifier from a format string char get_fmt_conversion(std::string_view fmt) { auto conversionSpecPos = fmt.find_first_of("diouxXfFeEgGaAcsp"); - // Check if a valid conversion specifier was found if (conversionSpecPos == std::string_view::npos) { return '\0'; // Return null character to indicate // no specifier found @@ -162,16 +160,6 @@ std::string print_part(const std::string& fmt, const char* data, size_t size) { return std::string(out.data()); } -char get_quote(const std::string& str) { - if (str.find('"') != std::string::npos) { - return '"'; - } - if (str.find('\'') != std::string::npos) { - return '\''; - } - return '\0'; -} - void process_printf(char*& data, const printf_descriptor_map_t& descs, char* data_end, cvk_printf_callback_t printf_cb, void* printf_userdata) { @@ -182,72 +170,40 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, std::stringstream printf_out{}; // Firstly print the part of the format string up to the first '%' + // if there is no % print the string as is. size_t next_part = format_string.find_first_of('%'); - if (next_part > format_string.size()) { + if (next_part < format_string.size()) { + printf_out << format_string.substr(0, next_part); + } else { + printf_out << format_string; + next_part = format_string.size(); data = data_end; } - printf_out << format_string.substr(0, next_part); // Decompose the remaining format string into individual strings with // one format specifier each, handle each one individually size_t arg_idx = 0; - std::stack quotes; - if (format_string.size() > 0) { - auto curr_quote = get_quote(format_string.substr( - 0, next_part)); // checking if curr selection has a " or '. - if (curr_quote) { - quotes.push(curr_quote); - } - } while (next_part < format_string.size() - 1) { // Get the part of the format string before the next format specifier size_t part_start = next_part; size_t part_end = format_string.find_first_of('%', part_start + 1); - - // Handle case where there's no second '%' - if (part_end == std::string::npos || part_end >= format_string.size()) { - part_end = - format_string.size(); // Set part_end to the end of the string - } auto part_fmt = format_string.substr(part_start, part_end - part_start); - auto curr_quote = get_quote(format_string.substr(part_start, part_end)); - if (curr_quote) { - if (!quotes.empty() && quotes.top() == curr_quote) { - quotes.pop(); - } else { - quotes.push(curr_quote); - } - } // Handle special cases - if (part_fmt == "%") { - printf_out << part_fmt; - // check for two consecutive % since otherwise the way we - // are moving the next_part var this will be skipped. - // printf requires two % i.e. %% to display % iff they are not - // at the start of the string. All starting %% shoulld be - // collapsed into one %. - // also check the the two consecutive % is not a case of %%s - // if the two %% are in quotes then we will collapse it into one % - auto curr_str = printf_out.str(); - if (curr_str.length() > 1 && quotes.empty() && - part_start + 1 < format_string.length() && - format_string[part_start + 1] == '%' && - !(part_start + 2 < format_string.length() && - format_string[part_start + 2] == 's')) { - printf_out << part_fmt; - } - next_part = part_end; - // Skip the next % as well since we have already added it. + if (part_end == part_start + 1) { if (format_string[part_start] == '%') { - next_part++; + printf_out << "%"; + // in cases of the last two chars % we need to add it twice. + if (part_end == format_string.size() - 2) { + printf_out << "%"; + } + next_part = part_end + 1; + } else { + // in this case we just print and move by 1. + printf_out << format_string[part_start]; + next_part = part_end; } continue; - // Single char - } else if (part_fmt.length() == 1) { - printf_out << part_fmt; - next_part = part_end; - continue; } else if (part_end == std::string::npos && arg_idx >= descs.at(printf_id).arg_sizes.size()) { // If there are no remaining arguments, the rest of the format @@ -258,19 +214,16 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // The size of the argument that this format part will consume size_t size = 0; - + // Updated if the size is valid if (descs.count(printf_id) > 0 && !descs.at(printf_id).arg_sizes.empty() && arg_idx < descs.at(printf_id).arg_sizes.size()) { size = descs.at(printf_id).arg_sizes[arg_idx]; } - // Handle invalid cases (size will be 0 here) - if (size == 0 || data + size > data_end) { + if (data + size > data_end) { data += size; - if (data > data_end) { - return; - } + return; } // Check to see if we have a vector format specifier @@ -284,9 +237,7 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Special case for %s if (get_fmt_conversion(part_fmt) == 's') { uint32_t string_id = read_buff(data); - if (string_id >= descs.size()) { - printf_out << ""; - } else { + if (string_id < descs.size()) { printf_out << print_part( part_fmt, descs.at(string_id).format_string.c_str(), size); @@ -320,10 +271,6 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, } auto output = printf_out.str(); - // special case for single % which needs to be represented by two %%. - if (output == "%") { - output = "%%"; - } if (printf_cb != nullptr) { auto len = output.size(); printf_cb(output.c_str(), len, data >= data_end, printf_userdata); From 852e14c0c685b42ff91b916c2821b842fb553684 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Tue, 1 Oct 2024 21:22:57 -0700 Subject: [PATCH 18/27] condense logic further --- src/printf.cpp | 65 ++++++++++++++++++++++---------------------------- 1 file changed, 28 insertions(+), 37 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index 1f449642..7e5d54f4 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -19,10 +19,6 @@ // Extract the conversion specifier from a format string char get_fmt_conversion(std::string_view fmt) { auto conversionSpecPos = fmt.find_first_of("diouxXfFeEgGaAcsp"); - if (conversionSpecPos == std::string_view::npos) { - return '\0'; // Return null character to indicate - // no specifier found - } return fmt.at(conversionSpecPos); } @@ -61,7 +57,7 @@ std::string get_vector_fmt(std::string fmt, int& vector_size, int& element_size, size_t vec_length_pos_start = ++pos; size_t vec_length_pos_end = - fmt.find_first_not_of("23468", vec_length_pos_start + 1); + fmt.find_first_not_of("23468", vec_length_pos_start); auto vec_length_str = fmt.substr(vec_length_pos_start, vec_length_pos_end - vec_length_pos_start); int vec_length = std::atoi(vec_length_str.c_str()); @@ -115,30 +111,32 @@ std::string print_part(const std::string& fmt, const char* data, size_t size) { case 'e': case 'g': case 'a': { - if (size == 2) + if (size == 2) { written = snprintf(out.data(), out_size, fmt.c_str(), cl_half_to_float(read_buff(data))); - else if (size == 4) + } else if (size == 4) { written = snprintf(out.data(), out_size, fmt.c_str(), read_buff(data)); - else + } else { written = snprintf(out.data(), out_size, fmt.c_str(), read_buff(data)); + } break; } default: { - if (size == 1) + if (size == 1) { written = snprintf(out.data(), out_size, fmt.c_str(), read_buff(data)); - else if (size == 2) + } else if (size == 2) { written = snprintf(out.data(), out_size, fmt.c_str(), read_buff(data)); - else if (size == 4) + } else if (size == 4) { written = snprintf(out.data(), out_size, fmt.c_str(), read_buff(data)); - else + } else { written = snprintf(out.data(), out_size, fmt.c_str(), read_buff(data)); + } break; } } @@ -169,21 +167,20 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, std::stringstream printf_out{}; - // Firstly print the part of the format string up to the first '%' - // if there is no % print the string as is. + // Firstly print the part of the format string up to the first '%' if any + // otherwise print the whole string as is and move the data pointer to the + // end. size_t next_part = format_string.find_first_of('%'); - if (next_part < format_string.size()) { - printf_out << format_string.substr(0, next_part); - } else { + if (next_part == std::string::npos) { printf_out << format_string; - next_part = format_string.size(); data = data_end; + } else { + printf_out << format_string.substr(0, next_part); } - // Decompose the remaining format string into individual strings with // one format specifier each, handle each one individually size_t arg_idx = 0; - while (next_part < format_string.size() - 1) { + while (next_part < format_string.size()) { // Get the part of the format string before the next format specifier size_t part_start = next_part; size_t part_end = format_string.find_first_of('%', part_start + 1); @@ -191,17 +188,17 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Handle special cases if (part_end == part_start + 1) { - if (format_string[part_start] == '%') { - printf_out << "%"; - // in cases of the last two chars % we need to add it twice. - if (part_end == format_string.size() - 2) { - printf_out << "%"; - } - next_part = part_end + 1; + printf_out << "%"; + size_t next_format = format_string.find_first_of('%', part_end + 1); + // Check if there is anything to print between two % + if (next_format != std::string::npos && + next_format - part_end > 1) { + // Print everything in between the two % + printf_out << format_string.substr( + part_end + 1, next_format - (part_end + 1)); + next_part = next_format; } else { - // in this case we just print and move by 1. - printf_out << format_string[part_start]; - next_part = part_end; + next_part = part_end + 1; } continue; } else if (part_end == std::string::npos && @@ -213,13 +210,7 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, } // The size of the argument that this format part will consume - size_t size = 0; - // Updated if the size is valid - if (descs.count(printf_id) > 0 && - !descs.at(printf_id).arg_sizes.empty() && - arg_idx < descs.at(printf_id).arg_sizes.size()) { - size = descs.at(printf_id).arg_sizes[arg_idx]; - } + auto& size = descs.at(printf_id).arg_sizes[arg_idx]; if (data + size > data_end) { data += size; From 49fc753448a10f9dd062246f3d0e76365736682e Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Tue, 1 Oct 2024 21:29:34 -0700 Subject: [PATCH 19/27] revert untouched code --- src/printf.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index 7e5d54f4..eaa498c0 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -111,32 +111,30 @@ std::string print_part(const std::string& fmt, const char* data, size_t size) { case 'e': case 'g': case 'a': { - if (size == 2) { + if (size == 2) written = snprintf(out.data(), out_size, fmt.c_str(), cl_half_to_float(read_buff(data))); - } else if (size == 4) { + else if (size == 4) written = snprintf(out.data(), out_size, fmt.c_str(), read_buff(data)); - } else { + else written = snprintf(out.data(), out_size, fmt.c_str(), read_buff(data)); - } break; } default: { - if (size == 1) { + if (size == 1) written = snprintf(out.data(), out_size, fmt.c_str(), read_buff(data)); - } else if (size == 2) { + else if (size == 2) written = snprintf(out.data(), out_size, fmt.c_str(), read_buff(data)); - } else if (size == 4) { + else if (size == 4) written = snprintf(out.data(), out_size, fmt.c_str(), read_buff(data)); - } else { + else written = snprintf(out.data(), out_size, fmt.c_str(), read_buff(data)); - } break; } } From 181a82cd8680e8ff22a5bd2f02b744e0fc4d54e9 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Date: Wed, 2 Oct 2024 07:01:46 -0700 Subject: [PATCH 20/27] commit romaric's suggestion Co-authored-by: Romaric Jodin --- src/printf.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index eaa498c0..02eb6c01 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -187,16 +187,14 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Handle special cases if (part_end == part_start + 1) { printf_out << "%"; - size_t next_format = format_string.find_first_of('%', part_end + 1); - // Check if there is anything to print between two % - if (next_format != std::string::npos && - next_format - part_end > 1) { - // Print everything in between the two % - printf_out << format_string.substr( - part_end + 1, next_format - (part_end + 1)); - next_part = next_format; - } else { - next_part = part_end + 1; + // We also need to print the literals between '%%' and the next '%' + next_part = part_start = part_end + 1; + part_end = format_string.find_first_of('%', part_start); + if (part_end != std::string::npos && part_end > part_start) { + part_fmt = + format_string.substr(part_start, part_end - part_start); + printf_out << part_fmt; + next_part = part_end; } continue; } else if (part_end == std::string::npos && From 41c9d7943213a82e1016577b2c20b39d7d0e8f20 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Thu, 3 Oct 2024 10:53:12 -0700 Subject: [PATCH 21/27] remove uneeded checks --- src/printf.cpp | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index 02eb6c01..235edff8 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -170,11 +170,11 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // end. size_t next_part = format_string.find_first_of('%'); if (next_part == std::string::npos) { - printf_out << format_string; + next_part = format_string.size(); data = data_end; - } else { - printf_out << format_string.substr(0, next_part); } + printf_out << format_string.substr(0, next_part); + // Decompose the remaining format string into individual strings with // one format specifier each, handle each one individually size_t arg_idx = 0; @@ -224,11 +224,8 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Special case for %s if (get_fmt_conversion(part_fmt) == 's') { uint32_t string_id = read_buff(data); - if (string_id < descs.size()) { - printf_out << print_part( - part_fmt, descs.at(string_id).format_string.c_str(), - size); - } + printf_out << print_part( + part_fmt, descs.at(string_id).format_string.c_str(), size); } else { printf_out << print_part(part_fmt, data, size); } From 39dda984661771fc648ca9a0073074ef846f98fd Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Thu, 3 Oct 2024 11:24:35 -0700 Subject: [PATCH 22/27] restore the condition --- src/printf.cpp | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index 235edff8..bc6161b8 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -187,14 +187,16 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Handle special cases if (part_end == part_start + 1) { printf_out << "%"; - // We also need to print the literals between '%%' and the next '%' - next_part = part_start = part_end + 1; - part_end = format_string.find_first_of('%', part_start); - if (part_end != std::string::npos && part_end > part_start) { - part_fmt = - format_string.substr(part_start, part_end - part_start); - printf_out << part_fmt; - next_part = part_end; + size_t next_format = format_string.find_first_of('%', part_end + 1); + // Check if there is anything to print between two % + if (next_format != std::string::npos && + next_format - part_end > 1) { + // Print everything in between the two % + printf_out << format_string.substr( + part_end + 1, next_format - (part_end + 1)); + next_part = next_format; + } else { + next_part = part_end + 1; } continue; } else if (part_end == std::string::npos && @@ -224,8 +226,11 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Special case for %s if (get_fmt_conversion(part_fmt) == 's') { uint32_t string_id = read_buff(data); - printf_out << print_part( - part_fmt, descs.at(string_id).format_string.c_str(), size); + if (string_id < descs.size()) { + printf_out << print_part( + part_fmt, descs.at(string_id).format_string.c_str(), + size); + } } else { printf_out << print_part(part_fmt, data, size); } From 9c25fbe8fdcea7cac7ff2c7e9f8be47f4130b805 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Mon, 7 Oct 2024 09:12:56 -0700 Subject: [PATCH 23/27] get rid of condition again --- src/printf.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index bc6161b8..619e8644 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -226,11 +226,8 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Special case for %s if (get_fmt_conversion(part_fmt) == 's') { uint32_t string_id = read_buff(data); - if (string_id < descs.size()) { - printf_out << print_part( - part_fmt, descs.at(string_id).format_string.c_str(), - size); - } + printf_out << print_part( + part_fmt, descs.at(string_id).format_string.c_str(), size); } else { printf_out << print_part(part_fmt, data, size); } From 6d85e0c68ea66576816c612552d0093fa3aab408 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Date: Tue, 8 Oct 2024 08:19:10 -0700 Subject: [PATCH 24/27] Add romaric's suggestion back in Co-authored-by: Romaric Jodin --- src/printf.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/printf.cpp b/src/printf.cpp index 619e8644..235edff8 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -187,16 +187,14 @@ void process_printf(char*& data, const printf_descriptor_map_t& descs, // Handle special cases if (part_end == part_start + 1) { printf_out << "%"; - size_t next_format = format_string.find_first_of('%', part_end + 1); - // Check if there is anything to print between two % - if (next_format != std::string::npos && - next_format - part_end > 1) { - // Print everything in between the two % - printf_out << format_string.substr( - part_end + 1, next_format - (part_end + 1)); - next_part = next_format; - } else { - next_part = part_end + 1; + // We also need to print the literals between '%%' and the next '%' + next_part = part_start = part_end + 1; + part_end = format_string.find_first_of('%', part_start); + if (part_end != std::string::npos && part_end > part_start) { + part_fmt = + format_string.substr(part_start, part_end - part_start); + printf_out << part_fmt; + next_part = part_end; } continue; } else if (part_end == std::string::npos && From e7936e42d008f05493251e7965c9e2f7342a9dd6 Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Wed, 16 Oct 2024 12:50:41 -0700 Subject: [PATCH 25/27] add two tests for new functionality --- src/printf.cpp | 2 +- tests/api/printf.cpp | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/printf.cpp b/src/printf.cpp index 235edff8..4eb4ff9b 100644 --- a/src/printf.cpp +++ b/src/printf.cpp @@ -43,7 +43,7 @@ std::string get_vector_fmt(std::string fmt, int& vector_size, int& element_size, // Consume precision and field width pos = fmt.find_first_not_of("123456789.", pos); - if (fmt.at(pos) != 'v') { + if (pos == std::string::npos || fmt.at(pos) != 'v') { vector_size = 1; return std::string{fmt}; } diff --git a/tests/api/printf.cpp b/tests/api/printf.cpp index 58127a05..27f02d7e 100644 --- a/tests/api/printf.cpp +++ b/tests/api/printf.cpp @@ -23,6 +23,41 @@ TEST_F(WithCommandQueueAndPrintf, SimplePrintf) { char source[512]; sprintf(source, "kernel void test_printf() { printf(\"%s\");}", message); auto kernel = CreateKernel(source, "test_printf"); + size_t gws = 1; + size_t lws = 1; + EnqueueNDRangeKernel(kernel, 1, nullptr, &gws, &lws, 0, nullptr, nullptr); + Finish(); + ASSERT_STREQ(m_printf_output.c_str(), message); +} + +TEST_F(WithCommandQueueAndPrintf, SimplePrintfPercent) { + // Tests that the while loop inside the process_printf func + // goes all the way to the end of the string to be printed. + const char message[] = "The value is: 123%"; + char source[512]; + sprintf(source, + "kernel void test_printf() { printf(\"The value is: %d%\"); }", + 123); + + auto kernel = CreateKernel(source, "test_printf"); + + size_t gws = 1; + size_t lws = 1; + EnqueueNDRangeKernel(kernel, 1, nullptr, &gws, &lws, 0, nullptr, nullptr); + Finish(); + + ASSERT_STREQ(m_printf_output.c_str(), message); +} + +TEST_F(WithCommandQueueAndPrintf, SimplePrintfBetweenPercents) { + const char message[] = "%Hello%World%!"; + const char message_test[] = "%%Hello%%World%%!"; + + char source[512]; + sprintf(source, "kernel void test_printf() { printf(\"%s\");}", + message_test); + + auto kernel = CreateKernel(source, "test_printf"); size_t gws = 1; size_t lws = 1; From c0a381c8e83a2a2809d81af58932466e1adfad5a Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Wed, 16 Oct 2024 13:54:02 -0700 Subject: [PATCH 26/27] add one last test --- tests/api/printf.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/api/printf.cpp b/tests/api/printf.cpp index 27f02d7e..8cd4e90d 100644 --- a/tests/api/printf.cpp +++ b/tests/api/printf.cpp @@ -78,6 +78,20 @@ TEST_F(WithCommandQueueAndPrintf, SimpleFormatedPrintf) { ASSERT_STREQ(m_printf_output.c_str(), ""); } +TEST_F(WithCommandQueueAndPrintf, PrintfWithNoFormatSpecifier) { + const char* source = + "kernel void test_printf() { printf(\"\\n\", \"foo\");}"; + auto kernel = CreateKernel(source, "test_printf"); + + size_t gws = 1; + size_t lws = 1; + EnqueueNDRangeKernel(kernel, 1, nullptr, &gws, &lws, 0, nullptr, nullptr); + Finish(); + + // The expected output is just a newline character since there's no + // format specifier to consume the "foo" argument. + ASSERT_STREQ(m_printf_output.c_str(), "\n"); +} TEST_F(WithCommandQueueAndPrintf, TooLongPrintf) { // each print takes 12 bytes (4 for the printf_id, and 2*4 for the 2 integer From b3a51598473c79aa9481c3ff9856196538d2bfba Mon Sep 17 00:00:00 2001 From: Syed Faaiz Hussain Date: Mon, 21 Oct 2024 00:56:00 -0700 Subject: [PATCH 27/27] fix test for while loop --- tests/api/printf.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/api/printf.cpp b/tests/api/printf.cpp index 8cd4e90d..e9db7093 100644 --- a/tests/api/printf.cpp +++ b/tests/api/printf.cpp @@ -36,7 +36,7 @@ TEST_F(WithCommandQueueAndPrintf, SimplePrintfPercent) { const char message[] = "The value is: 123%"; char source[512]; sprintf(source, - "kernel void test_printf() { printf(\"The value is: %d%\"); }", + "kernel void test_printf() { printf(\"The value is: %d%%\"); }", 123); auto kernel = CreateKernel(source, "test_printf");