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

test: add usage of rtcp_msg_print() #1105

Merged
merged 12 commits into from
Apr 29, 2024
Merged

test: add usage of rtcp_msg_print() #1105

merged 12 commits into from
Apr 29, 2024

Conversation

alfredh
Copy link
Contributor

@alfredh alfredh commented Apr 25, 2024

Add usage of rtcp debug functions in testcode, to increase test coverage.

@alfredh
Copy link
Contributor Author

alfredh commented Apr 25, 2024

Some jobs are stuck in the deadlock:

  test 98 -- test_rtmps_publish
  test 99 -- test_rtp
  test 100 -- test_rtpext
  test 101 -- test_rtcp_encode  <--- Stuck here

@alfredh
Copy link
Contributor Author

alfredh commented Apr 26, 2024

it looks like the bitfields are promoted to int when printed:

	/** RTCP Header */
	struct rtcp_hdr {
		unsigned int version:2;  /**< Protocol version       */
		unsigned int p:1;        /**< Padding flag           */
		unsigned int count:5;    /**< Varies by packet type  */
		unsigned int pt:8;       /**< RTCP packet type       */
		uint16_t length;         /**< Packet length in words */
	} hdr;

@alfredh alfredh marked this pull request as draft April 26, 2024 08:02
@alfredh
Copy link
Contributor Author

alfredh commented Apr 27, 2024

hi @sreimers

It looks like there is a problem with gcc and printing of bitfields.

In this code when printing bitfields, the _Generic hits the default case:

#define RE_ARG_SIZE(type)                                                     \
	_Generic((type),                                                      \
	bool:			sizeof(int),                                  \
	char:			sizeof(int),                                  \
	unsigned char:		sizeof(unsigned int),                         \
	short:			sizeof(int),                                  \
	unsigned short:		sizeof(unsigned int),	                      \
	int:			sizeof(int),                                  \
	unsigned int:		sizeof(unsigned int),                         \
	long:			sizeof(long),                                 \
	unsigned long:		sizeof(unsigned long),                        \
	long long:		sizeof(long long),                            \
	unsigned long long:	sizeof(unsigned long long),                   \
	float:			sizeof(double),                               \
	double:			sizeof(double),                               \
	char const*:		sizeof(char const *),                         \
	char*:			sizeof(char *),                               \
	void const*:		sizeof(void const *),                         \
	void*:			sizeof(void *),                               \
	struct pl:		sizeof(struct pl),                            \
	default: sizeof(void*)                                                \

Here is a simple test program to repro the case.
Please note than when using printf there are no compiler warnings.

#include <stdio.h>
#include <inttypes.h>
#include <re/re.h>


int main()
{
	struct rtcp_hdr hdr;

	hdr.version = 1;
	hdr.p = 1;
	hdr.count = 2;
	hdr.pt = 200;
	hdr.length = 3;

	re_printf("%8s pad=%d count=%-2d pt=%-3d len=%u \n",
	       rtcp_type_name((enum rtcp_type)hdr.pt),
	       hdr.p,
	       hdr.count,
	       hdr.pt,
	       hdr.length);
}

@sreimers
Copy link
Member

sreimers commented Apr 27, 2024

The default promotion for variadic function args and small data types (bool, char, short) should be int or maybe higher, so yes this is wrong, unsure how to detect bit-fields nicely. I have to think about that.

@alfredh
Copy link
Contributor Author

alfredh commented Apr 27, 2024

One option is to stop using bitfields, it seems to be compiler specific...

@sreimers
Copy link
Member

I think I found a good workaround for bit fields: #1110

src/fmt/print.c Outdated
re_assert(0 && "RE_VA_ARG: no more arguments");
}
if (err == EOVERFLOW) {
DEBUG_WARNING("Format: \"%b<-- SIZE ERROR\n", fmt,
re_fprintf(stderr, "Format: \"%b<-- SIZE ERROR\n", fmt,
Copy link
Member

@sreimers sreimers Apr 28, 2024

Choose a reason for hiding this comment

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

Looks good, should we make a separat PR for this?

@alfredh alfredh marked this pull request as ready for review April 28, 2024 09:01
@alfredh
Copy link
Contributor Author

alfredh commented Apr 28, 2024

This one is now ready for merge...

@sreimers sreimers merged commit 06ce16c into main Apr 29, 2024
37 checks passed
@sreimers sreimers deleted the rtcp_msg_print branch April 29, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants