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

Compiling etw_logger_exporter.h with c++latest fails #3237

Open
jeremicmilan opened this issue Jan 9, 2025 · 4 comments
Open

Compiling etw_logger_exporter.h with c++latest fails #3237

jeremicmilan opened this issue Jan 9, 2025 · 4 comments
Assignees
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@jeremicmilan
Copy link

Issue description

Otel does not compile with c++latest. More specifically, file etw_logger_exporter.h is causing issues.

Repro steps

Extract the attached zip file. Open the folder in latest Visual Studio 2022. Configure should run automatically. After that build all with Ctrl+Shift+B. The build should fail if Clang preset is selected. MSVC preset works fine.

Example error output

>------ Build All started: Project: otel_repro, Configuration: windows-ninja-clang-x64-windows-ninja-clang-x64-debug ------
  [1/2] Building CXX object CMakeFiles\otel_repro.dir\Debug\otel_repro.cpp.obj
  FAILED: CMakeFiles/otel_repro.dir/Debug/otel_repro.cpp.obj 
  C:\PROGRA~1\MIB055~1\2022\ENTERP~1\VC\Tools\Llvm\x64\bin\clang-cl.exe  /nologo -TP -DABSL_CONSUME_DLL -DHAVE_ABSEIL -DHAVE_MSGPACK -DOPENTELEMETRY_ABI_VERSION_NO=1 -DCMAKE_INTDIR=\"Debug\" -imsvcC:\otel_repro\out\build\windows-ninja-clang-x64\vcpkg_installed\x64-windows\include --target=amd64-pc-windows-msvc -fdiagnostics-absolute-paths  /DWIN32 /D_WINDOWS /EHsc /Ob0 /Od /RTC1 -MDd -Zi -std:c++latest /showIncludes /FoCMakeFiles\otel_repro.dir\Debug\otel_repro.cpp.obj /FdCMakeFiles\otel_repro.dir\Debug\otel_repro.pdb -c -- C:\otel_repro\otel_repro.cpp
  In file included from C:\otel_repro\otel_repro.cpp:1:
  In file included from C:\otel_repro\out\build\windows-ninja-clang-x64\vcpkg_installed\x64-windows\include\opentelemetry\exporters\etw\etw_logger_exporter.h:23:
  In file included from C:\otel_repro\out\build\windows-ninja-clang-x64\vcpkg_installed\x64-windows\include\opentelemetry\exporters\etw\etw_config.h:13:
  In file included from C:\otel_repro\out\build\windows-ninja-clang-x64\vcpkg_installed\x64-windows\include\opentelemetry\exporters\etw\etw_provider.h:36:
  In file included from C:\otel_repro\out\build\windows-ninja-clang-x64\vcpkg_installed\x64-windows\include\opentelemetry\exporters\etw\etw_traceloggingdynamic.h:8:
C:\otel_repro\out\build\windows-ninja-clang-x64\vcpkg_installed\x64-windows\include\opentelemetry\exporters\etw\TraceLoggingDynamic.h(2144,31): error : invalid bitwise operation between different enumeration types ('Type' and 'tld::InType')
   2144 |             _tld_ASSERT((type & InTypeMask) == (type & 0xff), "InType out of range");
        |                          ~~~~ ^ ~~~~~~~~~~
  C:\otel_repro\out\build\windows-ninja-clang-x64\vcpkg_installed\x64-windows\include\opentelemetry\exporters\etw\TraceLoggingDynamic.h(653,45): note: expanded from macro '_tld_ASSERT'
    653 |     #define _tld_ASSERT(exp, str) ((void)(!(exp) ? (__annotation(L"Debug", L"AssertFail", L"TraceLogging: " L#exp L" : " L##str), DbgRaiseAssertionFailure(), 0) : 0))
        |                                             ^~~
C:\otel_repro\out\build\windows-ninja-clang-x64\vcpkg_installed\x64-windows\include\opentelemetry\exporters\etw\TraceLoggingDynamic.h(2144,31): error : invalid bitwise operation between different enumeration types ('Type' and 'tld::InType')
   2144 |             _tld_ASSERT((type & InTypeMask) == (type & 0xff), "InType out of range");
        |                          ~~~~ ^ ~~~~~~~~~~
  C:\otel_repro\out\build\windows-ninja-clang-x64\vcpkg_installed\x64-windows\include\opentelemetry\exporters\etw\TraceLoggingDynamic.h(653,45): note: expanded from macro '_tld_ASSERT'
    653 |     #define _tld_ASSERT(exp, str) ((void)(!(exp) ? (__annotation(L"Debug", L"AssertFail", L"TraceLogging: " L#exp L" : " L##str), DbgRaiseAssertionFailure(), 0) : 0))
        |                                             ^~~
  C:\otel_repro\out\build\windows-ninja-clang-x64\vcpkg_installed\x64-windows\include\opentelemetry\exporters\etw\TraceLoggingDynamic.h(2289,13): note: in instantiation of member function 'tld::EventMetadataBuilder<std::vector<unsigned char>>::AddFieldInfo' requested here
   2289 |             AddFieldInfo(InMetaScalar, type, fieldTags);
        |             ^
  C:\otel_repro\out\build\windows-ninja-clang-x64\vcpkg_installed\x64-windows\include\opentelemetry\exporters\etw\etw_provider.h(483,19): note: in instantiation of function template specialization 'tld::EventMetadataBuilder<std::vector<unsigned char>>::AddField<char>' requested here
    483 |           builder.AddField(name, tld::TypeBool8);
        |                   ^
  2 errors generated.
  ninja: build stopped: subcommand failed.

Build All failed.

Proposed fix

Do appropriate cast of different integer/enum types in first line of AddFieldInfo.

Current line:

_tld_ASSERT((type & InTypeMask) == (type & 0xff), "InType out of range");

Potential fix:

_tld_ASSERT((type & (Type)InTypeMask) == (type & 0xff), "InType out of range");
@jeremicmilan jeremicmilan added the bug Something isn't working label Jan 9, 2025
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 9, 2025
@ThomsonTan
Copy link
Contributor

@jeremicmilan adding typecast looks good if could fix the issue. Could you please upload a PR with the fix?

@lalitb
Copy link
Member

lalitb commented Jan 10, 2025

I think TraceLoggingDynamics.h is maintained in some other MS repo too, and it would be good to see if there are some latest commits there.

@jeremicmilan
Copy link
Author

@ThomsonTan, I can send out a PR with my proposed fix manually validating that it works. However, I do not have time to add a test for this. Is that ok?

@lalitb, I guess you're thinking about this file. There are changes in differences in these files (mostly comments and SAL), but not changes on the lines in question:

image

It seems that the other repo is using MSBuild, so the issue is probably not a problem there (as the issue I'm reporting happens for clang with c++latest only). What is the plan between these two files? Do they get merged from time to time and in which repo?

@marcalff marcalff added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 13, 2025
@lalitb
Copy link
Member

lalitb commented Jan 16, 2025

Do they get merged from time to time and in which repo?

@jeremicmilan Thanks for looking into this. I don't think there is proper maintenance of this header outside of this repo. It would be good to have fix as you proposed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

4 participants