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

MDEV-35599: Replace (u)llstr() with %llu/d #3724

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
2 changes: 1 addition & 1 deletion client/mysql.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5188,7 +5188,7 @@ static int com_status(String *, char *)
tee_fprintf(stdout, "Protocol version:\t%d\n", mysql_get_proto_info(&mysql));
tee_fprintf(stdout, "Connection:\t\t%s\n", mysql_get_host_info(&mysql));
if ((id= mysql_insert_id(&mysql)))
tee_fprintf(stdout, "Insert id:\t\t%s\n", llstr(id, buff));
tee_fprintf(stdout, "Insert id:\t\t%lld\n", id);

/* "limit 1" is protection against SQL_SELECT_LIMIT=0 */
if (mysql_real_query_for_lazy(C_STRING_WITH_LEN(
Expand Down
10 changes: 5 additions & 5 deletions client/mysqladmin.cc
Original file line number Diff line number Diff line change
Expand Up @@ -851,8 +851,10 @@ static int execute_commands(MYSQL *mysql,int argc, char **argv)
{
/* We don't use mysql_kill(), since it only handles 32-bit IDs. */
char buff[26], *out; /* "KILL " + max 20 digs + NUL */
out= strxmov(buff, "KILL ", NullS);
ullstr(strtoull(pos, NULL, 0), out);
out= strmov(buff, "KILL ");
// but first, also use `strtoull` to... truncate to ulonglong? //TODO: do we need to?
snprintf(buff, 21, "%llu",
(unsigned long long) strtoull(pos, NULL, 0)); // `strtoull` returns `ulong` on Linux!
ParadoxV5 marked this conversation as resolved.
Show resolved Hide resolved

if (mysql_query(mysql, buff))
{
Expand Down Expand Up @@ -1473,7 +1475,6 @@ static void print_row(MYSQL_RES *result, MYSQL_ROW cur,
static void print_relative_row(MYSQL_RES *result, MYSQL_ROW cur, uint row)
{
ulonglong tmp;
char buff[22];
MYSQL_FIELD *field;

mysql_field_seek(result, 0);
Expand All @@ -1482,8 +1483,7 @@ static void print_relative_row(MYSQL_RES *result, MYSQL_ROW cur, uint row)

field = mysql_fetch_field(result);
tmp = cur[1] ? strtoull(cur[1], NULL, 10) : (ulonglong) 0;
printf(" %-*s|\n", (int) field->max_length + 1,
llstr((tmp - last_values[row]), buff));
printf(" %-*lld|\n", (int) field->max_length + 1, tmp - last_values[row]);
last_values[row] = tmp;
}

Expand Down
13 changes: 4 additions & 9 deletions client/mysqlbinlog.cc
Original file line number Diff line number Diff line change
Expand Up @@ -715,7 +715,6 @@ static bool print_row_event(PRINT_EVENT_INFO *print_event_info, Log_event *ev,
Table_map_log_event *ignored_map=
print_event_info->m_table_map_ignored.get_table(table_id);
bool skip_event= (ignored_map != NULL);
char ll_buff[21];
bool result= 0;

if (opt_flashback)
Expand Down Expand Up @@ -821,8 +820,7 @@ static bool print_row_event(PRINT_EVENT_INFO *print_event_info, Log_event *ev,
if (is_stmt_end && !result)
{
if (print_event_info->print_row_count)
fprintf(result_file, "# Number of rows: %s\n",
llstr(print_event_info->row_events, ll_buff));
fprintf(result_file, "# Number of rows: %lld\n", print_event_info->row_events);
Copy link
Member

Choose a reason for hiding this comment

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

row_events is ulonglong, that is unsigned. Were you getting a compiler error here?

you wrote it's a non-goal, so I will no longer comment on signed/unsigned mismatch, but would the code compile without you fixing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it’s impractical to track typedefs manually when we can automatically catch them by turning on -Wformat-signedness, so I explicitly excluded this step.

From what I can tell, neither -Wformat nor -Wall include -Wformat-signedness, so issues like this continue not spawning compile warnings.

I mentioned -Wformat-signedness as a comment of MDEV-26896.
We can prioritize it here or we can push for MDEV-26896 to ship with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 Turning on -Wconversion (MDEV-26896) will complain about cases like these before this replacement.
🧐 It will also complain about char *ullstr(longlong value,char *buff) because it delegates a ulonglong arg to longlong10_to_str(value,buff,10); which is sign-agnostic.

print_event_info->row_events= 0;
}
return result;
Expand Down Expand Up @@ -863,7 +861,6 @@ static inline my_bool is_server_id_excluded(uint32 server_id)
Exit_status process_event(PRINT_EVENT_INFO *print_event_info, Log_event *ev,
my_off_t pos, const char *logname)
{
char ll_buff[21];
Log_event_type ev_type= ev->get_type_code();
my_bool destroy_evt= TRUE;
my_bool gtid_err= FALSE;
Expand Down Expand Up @@ -1040,7 +1037,7 @@ Exit_status process_event(PRINT_EVENT_INFO *print_event_info, Log_event *ev,
goto end;
}
if (print_row_event_positions)
fprintf(result_file, "# at %s\n",llstr(pos,ll_buff));
fprintf(result_file, "# at %lld\n", pos);

if (!opt_hexdump)
print_event_info->hexdump_from= 0; /* Disabled */
Expand Down Expand Up @@ -3160,7 +3157,6 @@ static Exit_status dump_local_log_entries(PRINT_EVENT_INFO *print_event_info,
}
for (;;)
{
char llbuff[21];
my_off_t old_off = my_b_tell(file);

Log_event* ev = Log_event::read_log_event(file, glob_description_event,
Expand All @@ -3175,9 +3171,8 @@ static Exit_status dump_local_log_entries(PRINT_EVENT_INFO *print_event_info,
file->error= 0;
else if (file->error)
{
error("Could not read entry at offset %s: "
"Error in log format or read error.",
llstr(old_off,llbuff));
error("Could not read entry at offset %lld: "
"Error in log format or read error.", old_off);
goto err;
}
// else file->error == 0 means EOF, that's OK, we break in this case
Expand Down
4 changes: 2 additions & 2 deletions client/mysqltest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7991,8 +7991,8 @@ void append_metadata(DYNAMIC_STRING *ds,
void append_info(DYNAMIC_STRING *ds, ulonglong affected_rows,
const char *info)
{
char buf[40], buff2[21];
size_t len= sprintf(buf,"affected rows: %s\n", llstr(affected_rows, buff2));
char buf[40];
size_t len= sprintf(buf,"affected rows: %lld\n", affected_rows);
dynstr_append_mem(ds, buf, len);
if (info)
{
Expand Down
2 changes: 0 additions & 2 deletions include/m_string.h
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,6 @@ size_t my_gcvt(double x, my_gcvt_arg_type type, int width, char *to,
*/
#define MY_GCVT_MAX_FIELD_WIDTH (DBL_DIG + 4 + MY_MAX(5, MAX_DECPT_FOR_F_FORMAT)) \

extern char *llstr(longlong value,char *buff);
extern char *ullstr(longlong value,char *buff);
#ifndef HAVE_STRTOUL
extern long strtol(const char *str, char **ptr, int base);
extern ulong strtoul(const char *str, char **ptr, int base);
Expand Down
6 changes: 2 additions & 4 deletions mysys/mf_iocache.c
Original file line number Diff line number Diff line change
Expand Up @@ -1920,7 +1920,6 @@ int main(int argc, char** argv)
MY_STAT status;
const char* fname="/tmp/iocache.test";
int cache_size=16384;
char llstr_buf[22];
int max_block,total_bytes=0;
int i,num_loops=100,error=0;
char *p;
Expand Down Expand Up @@ -1955,9 +1954,8 @@ int main(int argc, char** argv)
if (!my_stat(fname,&status,MYF(MY_WME)))
die("%s failed to stat, but I had just closed it,\
wonder how that happened");
printf("Final size of %s is %s, wrote %d bytes\n",fname,
llstr(status.st_size,llstr_buf),
total_bytes);
printf("Final size of %s is %lld, wrote %d bytes\n",
fname, status.st_size, total_bytes);
my_delete(fname, MYF(MY_WME));
/* check correctness of tests */
if (total_bytes != status.st_size)
Expand Down
16 changes: 6 additions & 10 deletions mysys/my_getopt.c
Original file line number Diff line number Diff line change
Expand Up @@ -1191,7 +1191,6 @@ longlong getopt_ll_limit_value(longlong num, const struct my_option *optp,
{
longlong old= num;
my_bool adjusted= FALSE;
char buf1[255], buf2[255];
ulonglong block_size= (optp->block_size ? (ulonglong) optp->block_size : 1L);
DBUG_ENTER("getopt_ll_limit_value");

Expand Down Expand Up @@ -1238,8 +1237,8 @@ longlong getopt_ll_limit_value(longlong num, const struct my_option *optp,
*fix= old != num;
else if (adjusted)
my_getopt_error_reporter(WARNING_LEVEL,
"option '%s': signed value %s adjusted to %s",
optp->name, llstr(old, buf1), llstr(num, buf2));
"option '%s': signed value %lld adjusted to %lld",
optp->name, old, num);
DBUG_RETURN(num);
}

Expand All @@ -1264,7 +1263,6 @@ ulonglong getopt_ull_limit_value(ulonglong num, const struct my_option *optp,
{
my_bool adjusted= FALSE;
ulonglong old= num;
char buf1[255], buf2[255];
DBUG_ENTER("getopt_ull_limit_value");

if ((ulonglong) num > (ulonglong) optp->max_value &&
Expand Down Expand Up @@ -1313,8 +1311,8 @@ ulonglong getopt_ull_limit_value(ulonglong num, const struct my_option *optp,
*fix= old != num;
else if (adjusted)
my_getopt_error_reporter(WARNING_LEVEL,
"option '%s': unsigned value %s adjusted to %s",
optp->name, ullstr(old, buf1), ullstr(num, buf2));
"option '%s': unsigned value %llu adjusted to %llu",
optp->name, old, num);

DBUG_RETURN(num);
}
Expand Down Expand Up @@ -1730,7 +1728,6 @@ void my_print_variables(const struct my_option *options)
{
uint name_space= 34, length, nr;
ulonglong llvalue;
char buff[255];
const struct my_option *optp;
DBUG_ENTER("my_print_variables");

Expand Down Expand Up @@ -1811,11 +1808,10 @@ void my_print_variables(const struct my_option *options)
printf("%lu\n", *((ulong*) value));
break;
case GET_LL:
printf("%s\n", llstr(*((longlong*) value), buff));
printf("%lld\n", *((longlong*) value));
break;
case GET_ULL:
longlong10_to_str(*((ulonglong*) value), buff, 10);
printf("%s\n", buff);
printf("%llu\n", *((ulonglong*) value));
break;
case GET_DOUBLE:
printf("%.10g\n", *(double*) value);
Expand Down
4 changes: 2 additions & 2 deletions sql/item_func.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4282,7 +4282,7 @@ longlong Item_func_get_lock::val_int()
if (args[1]->null_value)
strmov(buf, "NULL");
else
llstr(((longlong) timeout), buf);
snprintf(buf, sizeof(buf), "%lld", (longlong) timeout);
ParadoxV5 marked this conversation as resolved.
Show resolved Hide resolved
push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
ER_WRONG_VALUE_FOR_TYPE, ER(ER_WRONG_VALUE_FOR_TYPE),
"timeout", buf, "get_lock");
Expand Down Expand Up @@ -4523,7 +4523,7 @@ longlong Item_func_benchmark::val_int()
if (!args[0]->null_value)
{
char buff[22];
llstr(((longlong) loop_count), buff);
snprintf(buff, sizeof(buff), "%lld", (longlong) loop_count);
push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
ER_WRONG_VALUE_FOR_TYPE,
ER_THD(thd, ER_WRONG_VALUE_FOR_TYPE),
Expand Down
9 changes: 4 additions & 5 deletions sql/log.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3336,7 +3336,6 @@ bool MYSQL_QUERY_LOG::write(THD *thd, time_t current_time,
const char *sql_text, size_t sql_text_len)
{
bool error= 0;
char llbuff[22];
DBUG_ENTER("MYSQL_QUERY_LOG::write");

mysql_mutex_lock(&LOCK_log);
Expand Down Expand Up @@ -3422,15 +3421,15 @@ bool MYSQL_QUERY_LOG::write(THD *thd, time_t current_time,
if (thd->tmp_tables_used &&
my_b_printf(&log_file,
"# Tmp_tables: %lu Tmp_disk_tables: %lu "
"Tmp_table_sizes: %s\n",
"Tmp_table_sizes: %lld\n",
(ulong) thd->tmp_tables_used,
(ulong) thd->tmp_tables_disk_used,
llstr(thd->tmp_tables_size, llbuff)))
thd->tmp_tables_size))
goto err;
if (thd->max_tmp_space_used &&
my_b_printf(&log_file,
"# Max_tmp_disk_space_used: %s\n",
llstr(thd->max_tmp_space_used, llbuff)))
"# Max_tmp_disk_space_used: %lld\n",
thd->max_tmp_space_used))
goto err;
}

Expand Down
7 changes: 2 additions & 5 deletions sql/log.h
Original file line number Diff line number Diff line change
Expand Up @@ -936,13 +936,10 @@ class MYSQL_BIN_LOG: public TC_LOG, private Event_log
}
void harvest_bytes_written(Atomic_counter<uint64> *counter)
{
#ifdef DBUG_TRACE
char buf1[22],buf2[22];
#endif
DBUG_ENTER("harvest_bytes_written");
(*counter)+=bytes_written;
DBUG_PRINT("info",("counter: %s bytes_written: %s", llstr(*counter,buf1),
llstr(bytes_written,buf2)));
DBUG_PRINT("info",
("counter: %lld bytes_written: %lld", *counter, bytes_written));
bytes_written=0;
DBUG_VOID_RETURN;
}
Expand Down
39 changes: 13 additions & 26 deletions sql/log_event_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -334,14 +334,13 @@ bool Log_event::print_header(IO_CACHE* file,
PRINT_EVENT_INFO* print_event_info,
bool is_more __attribute__((unused)))
{
char llbuff[22];
my_off_t hexdump_from= print_event_info->hexdump_from;
DBUG_ENTER("Log_event::print_header");

if (my_b_write_byte(file, '#') ||
print_timestamp(file) ||
my_b_printf(file, " server id %lu end_log_pos %s ", (ulong) server_id,
llstr(log_pos,llbuff)))
my_b_printf(file, " server id %lu end_log_pos %llu ", (ulong) server_id,
log_pos))
goto err;

/* print the checksum */
Expand Down Expand Up @@ -1576,11 +1575,8 @@ bool Rows_log_event::print_verbose(IO_CACHE *file,

if (!(map= print_event_info->m_table_map.get_table(m_table_id)) ||
!(td= map->create_table_def()))
{
char llbuff[22];
return (my_b_printf(file, "### Row event for unknown table #%s",
ullstr(m_table_id, llbuff)));
}
return my_b_printf(file, "### Row event for unknown table #%llu",
m_table_id);

/* If the write rows event contained no values for the AI */
if (((general_type_code == WRITE_ROWS_EVENT) && (m_rows_buf==m_rows_end)))
Expand Down Expand Up @@ -2013,9 +2009,8 @@ bool Query_log_event::print_query_header(IO_CACHE* file,
(unlikely(print_event_info->sql_mode != sql_mode ||
!print_event_info->sql_mode_inited)))
{
char llbuff[22];
if (my_b_printf(file,"SET @@session.sql_mode=%s%s\n",
ullstr(sql_mode, llbuff), print_event_info->delimiter))
if (my_b_printf(file, "SET @@session.sql_mode=%llu%s\n",
sql_mode, print_event_info->delimiter))
Comment on lines +2012 to +2013
Copy link
Contributor Author

Choose a reason for hiding this comment

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

my_b_printf does not support and ignores %llu, causing segfaults here and likely erroneous outputs in other places.

my_b_printf is another one of our printf alternative that formats to my_b_write.
It only supports format specifiers %s, %c, %d/%u and %ld/%lu (yes, they’re specifiers in its lexicon) and ignores others.


Notably, my_b_printf also supports %b and the ` flag for %s similar to my_snprintf; but that’s switching to %sB and %sQ, whereas my_b_printf is a separate implementation independent of that.

When I migrated my_snprintf users, I recognized that my_b_printf isn’t part of the family and skipped over it.
That meant my_b_printf would remain incompatible with -Wformat and be “out of date” with the modified my_snprintf.

Except my_b_printf is causing trouble right now.
@vuvova, what shall we do about it?

Copy link
Member

Choose a reason for hiding this comment

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

suggestion — rewrite my_b_vprintf copying the logic from my_vfprintf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and they will be consistent until the next time we update the my_snprintf family.

🧐 I’m not sure about that. my_vfprintf works by resizing its allocation until the result fits1, whereas my_b_write writes to a limited buffer that only flushes when full.


Either case, I should mark this PR as on hold.

Footnotes

  1. … which is not efficient; unfortunately, unlike C/C++ snprintf, my_snprintf doesn’t tell the number of bytes it would’ve written if the output buffer’s sufficient.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I mean, my_b_vprintf could write to a buffer, resizing it like my_vfprintf does. Agree, it's not the best approach, but fixing it would be a separate task. At the end my_b_vprintf would use my_b_write to write the buffer to IO_CACHE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have another idea.

We can modify the implementation of my_vsnprintf to instead build around a writer callback, say bool (*writer)(void *reference, char *string, int string_length) that returns whether the loop should halt.
When respective buffers fill up, regular my_vsnprintf’s callback would return a halt request, while both my_vfprintf’s and my_b_vprintf’s would flush and carry on.

Of course, because my_vsnprintf is divided to many subfunction components, this approach is intrusive and very complex.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. That's why it wasn't implemented yet. In particular, it wasn't implemented, when we needed my_vfprintf.

A compromise approach — simpler than yours, more complex than what I suggested earlier — would be to make my_vsnprint_ex to return, as you wrote, number of bytes it would have written if the buffer were big enough. In this approach my_vsnprint can still return what it does now, it's a public API function and if we want to be conservative, we won't change it. But the internal my_vsnprint_ex can be changed to return whatever we want, and my_vfprintf can call it directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌 Let’s put then on the backburner for now; I have higher-priority assignments.

goto err;
print_event_info->sql_mode= sql_mode;
print_event_info->sql_mode_inited= 1;
Expand Down Expand Up @@ -2301,7 +2296,6 @@ bool Rotate_log_event::print(FILE* file, PRINT_EVENT_INFO* print_event_info)
if (print_event_info->short_form)
return 0;

char buf[22];
Write_on_release_cache cache(&print_event_info->head_cache, file,
Write_on_release_cache::FLUSH_F);
if (print_header(&cache, print_event_info, FALSE) ||
Expand All @@ -2310,7 +2304,7 @@ bool Rotate_log_event::print(FILE* file, PRINT_EVENT_INFO* print_event_info)
if (new_log_ident)
if (my_b_write(&cache, (uchar*) new_log_ident, (uint)ident_len))
goto err;
if (my_b_printf(&cache, " pos: %s\n", llstr(pos, buf)))
if (my_b_printf(&cache, " pos: %s\n", pos))
goto err;
return cache.flush_data();
err:
Expand Down Expand Up @@ -2374,7 +2368,6 @@ Gtid_list_log_event::print(FILE *file, PRINT_EVENT_INFO *print_event_info)

bool Intvar_log_event::print(FILE* file, PRINT_EVENT_INFO* print_event_info)
{
char llbuff[22];
const char *UNINIT_VAR(msg);
Write_on_release_cache cache(&print_event_info->head_cache, file,
Write_on_release_cache::FLUSH_F);
Expand All @@ -2400,8 +2393,7 @@ bool Intvar_log_event::print(FILE* file, PRINT_EVENT_INFO* print_event_info)
msg="INVALID_INT";
break;
}
if (my_b_printf(&cache, "%s=%s%s\n",
msg, llstr(val,llbuff), print_event_info->delimiter))
if (my_b_printf(&cache, "%s=%lld%s\n", msg, val, print_event_info->delimiter))
goto err;

return cache.flush_data();
Expand All @@ -2415,16 +2407,14 @@ bool Rand_log_event::print(FILE* file, PRINT_EVENT_INFO* print_event_info)
Write_on_release_cache cache(&print_event_info->head_cache, file,
Write_on_release_cache::FLUSH_F);

char llbuff[22],llbuff2[22];
if (!print_event_info->short_form)
{
if (print_header(&cache, print_event_info, FALSE) ||
my_b_write_string(&cache, "\tRand\n"))
goto err;
}
if (my_b_printf(&cache, "SET @@RAND_SEED1=%s, @@RAND_SEED2=%s%s\n",
llstr(seed1, llbuff),llstr(seed2, llbuff2),
print_event_info->delimiter))
if (my_b_printf(&cache, "SET @@RAND_SEED1=%lld, @@RAND_SEED2=%lld%s\n",
seed1, seed2, print_event_info->delimiter))
goto err;

return cache.flush_data();
Expand Down Expand Up @@ -2934,11 +2924,9 @@ bool Rows_log_event::print_helper(FILE *file,

if (!print_event_info->short_form)
{
char llbuff[22];

print_header(head, print_event_info, !last_stmt_event);
if (my_b_printf(head, "\t%s: table id %s%s\n",
name, ullstr(m_table_id, llbuff),
if (my_b_printf(head, "\t%s: table id %lld%s\n", name, m_table_id,
last_stmt_event ? " flags: STMT_END_F" : ""))
goto err;
}
Expand Down Expand Up @@ -3138,12 +3126,11 @@ bool Table_map_log_event::print(FILE *file, PRINT_EVENT_INFO *print_event_info)
{
if (!print_event_info->short_form)
{
char llbuff[22];

print_header(&print_event_info->head_cache, print_event_info, TRUE);
if (my_b_printf(&print_event_info->head_cache,
"\tTable_map: %`s.%`s mapped to number %s%s\n",
m_dbnam, m_tblnam, ullstr(m_table_id, llbuff),
"\tTable_map: %`s.%`s mapped to number %llu%s\n",
m_dbnam, m_tblnam, m_table_id,
((m_flags & TM_BIT_HAS_TRIGGERS_F) ?
" (has triggers)" : "")))
goto err;
Expand Down
Loading