[Tarantool-patches] [PATCH] say: fix format for fiber()->fid

[Vladislav Shpilevoy]

>>> diff --git a/src/lib/core/say.c b/src/lib/core/say.c
>>> index cbd10e107..6138752a7 100644
>>> --- a/src/lib/core/say.c
>>> +++ b/src/lib/core/say.c
>>> @@ -792,7 +792,7 @@ say_format_plain_tail(char *buf, int len, int level, const char *filename,
>>>  	if (cord) {
>>>  		SNPRINT(total, snprintf, buf, len, " %s", cord->name);
>>>  		if (fiber() && fiber()->fid != FIBER_ID_SCHED) {
>>> -			SNPRINT(total, snprintf, buf, len, "/%i/%s",
>>> +			SNPRINT(total, snprintf, buf, len, "/%u/%s",
>>>  				fiber()->fid, fiber_name(fiber()));
>>
>> I remember we had some issues about %u not being compatible with uint32_t so
>> we did manual casts to 'unsigned'. Not sure though what was the commit/ticket,
>> or if it really happened.
> 
> I suspect you rather mean uint64_t, which indeed better convert to
> excplicit "long long".

No, I mean exactly uint32_t. You don't have a guarantee that it is
the same as 'unsigned' AFAIK.

> Strictly speaking if we really need to cover
> compatibility we would _had_ to use PRIx macros which are so ugly :/

This is why I want us to use casts to 'unsigned' when we use %u. They
are less ugly.

> So no, uint32_t is known to be safe with %u format.

Is it written in the standard somewhere? I honestly don't know.

> Side note: I found that we still use %u for some of uint64_t,
> in particuar
> 
> 	if (req->term < raft->volatile_term) {
> 		say_info("RAFT: the message is ignored due to outdated term - "
> 			 "current term is %u", raft->volatile_term);
> 		return 0;
> 	}
> 
> probably should use %llu and explicit "long long" for long standing
> servers where raft term would overflow 32 bit value.

Yes, looks like a bug.

But a more appropriate fix would be to investigate why don't we have
compile time checks for this. The bug in raft's say_info() is clearly
an error and should have been alarmed at compilation.

At least clang supports an attribute which you can set for a function
and get printf-like checks and warnings in case of format issues.

We should adopt this: https://clang.llvm.org/docs/AttributeReference.html#id278

Somewhy I thought we already use it, but now I don't see any usages.
It is easy to add, and helps to catch some weird stuff.

Lets do it in scope of this patch series to fix the issue entirely. Not
just in one exploded place.

>> I am fine with not using the cast as long as all platforms and compilers
>> build flawlessly. Also it is ok for me if you will add an explicit cast in
>> the next version of the patch. Up to you. The same below.
> 
> No, Vlad, I think we don't need explicit casts for 32bit values.

Do you have a proof for this? That it is safe. Where in the standard do they
say that '%u' is exactly 4 bytes?

> I pushed
> an update to the branch gorcunov/gh-5846-fid-name and here is a new
> changelog. Does it look better?

It seems you didn't really read my comment, did you? I said:

	"Please, add a changelog file"

I didn't ask for more info in the commit message. This particular
bug in this particular line was obvious.

But you didn't add a changelog **file**. It should be added here:
https://github.com/tarantool/tarantool/tree/master/changelogs/unreleased


After more consideration I also realized we should not use uint32_t
for fiber ids. It is monotonically growing. Which means it will overflow
in the foreseeable future on a long-running instance.

For example, assume one fiber is started and recycled each millisecond
(which is quite a small rate). Then uint32 will overflow in less than
50 days. Considering we have instances running for years (at least from
what I heard from Mons), this is clearly a bug.


On the summary, it seems we need to

- Add compile-time format checks using clang __format__ attribute;
- Fix all the wrong formats;
- Use 64bit ids for fibers. Unless I am wrong somewhere in my thoughts
  above.