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

tests: internal: log_event_encoder: Add test code #7736

Closed
wants to merge 5 commits into from

Conversation

nokute78
Copy link
Collaborator

This is a refactor PR for #7584

This patch is to add test code for log_event_encoder.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • Run local packaging test showing all targets (including any new ones) build.
  • Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

Backporting

  • Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@nokute78 nokute78 temporarily deployed to pr July 23, 2023 01:50 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr July 23, 2023 01:50 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr July 23, 2023 01:50 — with GitHub Actions Inactive
@nokute78 nokute78 force-pushed the 2_test_log_event_encoder branch from 654020e to 205f2e7 Compare July 23, 2023 02:11
@nokute78 nokute78 temporarily deployed to pr July 23, 2023 02:11 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr July 23, 2023 02:11 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr July 23, 2023 02:11 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr July 23, 2023 02:38 — with GitHub Actions Inactive
@nokute78 nokute78 force-pushed the 2_test_log_event_encoder branch from 205f2e7 to 6c71b38 Compare July 23, 2023 02:49
@nokute78 nokute78 temporarily deployed to pr July 23, 2023 02:50 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr July 23, 2023 02:50 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr July 23, 2023 02:50 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr July 23, 2023 03:18 — with GitHub Actions Inactive
@nokute78
Copy link
Collaborator Author

Windows CI on x86_64 failed...

@nokute78
Copy link
Collaborator Author

nokute78 commented Jul 30, 2023

I tested following minimum test code.
It will work on windows x64 using /Od that means no compilation optimization.
On the other hand, it will not work on windows x64 using /O2.

#include <stdio.h>
#include <stdarg.h>
#include <string.h>

void Test(int loop_cnt, ...)
{
    va_list arg;
    int type1;
    int type2;
    char* str;
    size_t len1;
    size_t len2;
    int i;
    va_start(arg, loop_cnt);

    for (i = 0; i < loop_cnt; i++) {
        printf("i=%d\n", i);

        type1 = va_arg(arg, int);
        if (type1 != 1) {
            printf("%d:type is not 1\n", i);
        }

        len1 = va_arg(arg, size_t);
        if (len1 > 20) {
            printf("%d: len error. len=%zu\n", i, len1);
        }

        type2 = va_arg(arg, int);
        if (type2 != 2) {
            printf("%d:type is not 2\n", i);
        }

        str = va_arg(arg, char*);
        len2 = va_arg(arg, size_t);
        if (len2 != strlen(str)) {
            printf("%d: len error2. len=%zu\n", i, len2);
        }

        if (len1 != len2) {
            printf("%d: len error3. len1=%zu len2=%zu\n", i, len1, len2);
        }
    }
    va_end(arg);
}

int main(void) {
    int cnt = 2;

    Test(cnt,
        (int)1, (size_t)strlen("key1"), (int)2, (char*)"key1", (size_t)strlen("key1"),
        (int)1, (size_t)strlen("value2"), (int)2, (char*)"value2", (size_t)strlen("value2"));
    return 0;
}

@nokute78
Copy link
Collaborator Author

nokute78 commented Aug 4, 2023

I checked assembly code and it passed strlen("value2") using mov dword ptr [rsp+50h],6.
dword means 4bytes. I think it should be qword since size_t is unsigned __int64 on Windows x86_64.
https://learn.microsoft.com/en-us/cpp/c-runtime-library/standard-types?view=msvc-170

@nokute78 nokute78 temporarily deployed to pr August 5, 2023 01:58 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr August 5, 2023 01:58 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr August 5, 2023 01:58 — with GitHub Actions Inactive
Windows x86_64 passes dword size_t and it causes encoder error.
Since size_t should be unsigned __int64.

We define dword flb_log_event_size_t to prevent invalid casting as a workaround.

Signed-off-by: Takahiro Yamashita <[email protected]>
@nokute78 nokute78 force-pushed the 2_test_log_event_encoder branch from ea267be to 2485d34 Compare August 5, 2023 02:03
@nokute78 nokute78 temporarily deployed to pr August 5, 2023 02:03 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr August 5, 2023 02:03 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr August 5, 2023 02:03 — with GitHub Actions Inactive
@nokute78 nokute78 temporarily deployed to pr August 5, 2023 02:31 — with GitHub Actions Inactive
@nokute78
Copy link
Collaborator Author

nokute78 commented Aug 5, 2023

@edsiper @leonardo-albertovich Wow CI on Windows x64 passed!

I defined uint32_t type as an alternate size_t on Windows x64 for workaround.
Since fluent-bit that is compiled using /O2 passes dword as size_t and it causes a bug.

I also tried to define uint64_t , but it didn't work correctly.

@nokute78
Copy link
Collaborator Author

nokute78 commented Sep 9, 2023

ping

@leonardo-albertovich
Copy link
Collaborator

Hi @nokute78, I have this on my queue, I'll independently verify the claims in your PR using your test case in master because I'm not quite convinced about it.

I'll get back to you as soon as I'm able to.

@nokute78
Copy link
Collaborator Author

nokute78 commented Sep 9, 2023

@leonardo-albertovich Thank you.

The point is following code will cause runtime error using MSVC + /O2 option.
#7736 (comment)
It is the reason failing CI on Windows x64.

The code is to imitate a following macro FLB_LOG_EVENT_CSTRING_VALUE.

    ret = flb_log_event_encoder_append_body_values(
                &encoder,
                FLB_LOG_EVENT_CSTRING_VALUE("key1"),
                FLB_LOG_EVENT_CSTRING_VALUE("value2"),

MSVC outputs an assembly mov dword ptr [rsp+50h],6 for strlen("value2").
dword is 4bytes , but size_t should be 8bytes on Windows x64.

@leonardo-albertovich
Copy link
Collaborator

I have opened PR #7971 to fix this issue, once it's merged you can rebase and revert the rest of the changes so this PR only adds the test case.

I have verified the PR with your test case and it works as expected.

The issue is not that msvc defines size_t as a DWORD, that goes against the standard, the issue is that it's not respecting the cast and promotion rules which probably stems from the level of indirection and variadic function.

@leonardo-albertovich
Copy link
Collaborator

You can revert the rest of the modifications and rebase from master now leaving only the new test case.

@nokute78
Copy link
Collaborator Author

I created a new PR #7992

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants