Skip to content

Fix string literal conditions used in OPENVINO_ASSERT#33911

Open
aobolensk wants to merge 25 commits intoopenvinotoolkit:masterfrom
aobolensk:core-string-lit-assert
Open

Fix string literal conditions used in OPENVINO_ASSERT#33911
aobolensk wants to merge 25 commits intoopenvinotoolkit:masterfrom
aobolensk:core-string-lit-assert

Conversation

@aobolensk
Copy link
Contributor

@aobolensk aobolensk commented Jan 30, 2026

Details:

Fix "always true" assertions when string literal is passed as an assert argument.

Tickets:

  • N/A

@aobolensk aobolensk requested review from a team as code owners January 30, 2026 16:51
@github-actions github-actions bot added category: Core OpenVINO Core (aka ngraph) category: GPU OpenVINO GPU plugin category: HETERO OpenVINO HETERO plugin category: transformations OpenVINO Runtime library - Transformations category: CPP API OpenVINO CPP API bindings labels Jan 30, 2026
@aobolensk aobolensk force-pushed the core-string-lit-assert branch from 0940e67 to e47d20b Compare January 30, 2026 16:59
@aobolensk aobolensk requested review from a team as code owners January 30, 2026 16:59
@github-actions github-actions bot added the category: CPU OpenVINO CPU plugin label Jan 30, 2026
@aobolensk aobolensk force-pushed the core-string-lit-assert branch 2 times, most recently from 07e770d to d09b6a8 Compare January 30, 2026 17:30
@aobolensk aobolensk requested a review from a team as a code owner January 30, 2026 17:30
@github-actions github-actions bot added the category: LP transformations OpenVINO Low Precision transformations label Jan 30, 2026
@aobolensk aobolensk force-pushed the core-string-lit-assert branch 2 times, most recently from 5bc283a to c522f11 Compare January 30, 2026 18:15
@aobolensk aobolensk changed the title [Core][GPU][Hetero] Add compile-time guard to prevent string-literal conditions in OPENVINO_ASSERT Add compile time guard to prevent string literal conditions in OPENVINO_ASSERT Jan 30, 2026
…conditions in OPENVINO_ASSERT

Add guard and update hetero/gpu code paths to use OPENVINO_THROW or explicit predicate asserts
@aobolensk aobolensk force-pushed the core-string-lit-assert branch from 277aecc to c649f8f Compare February 2, 2026 12:12
Copy link
Contributor

@Lyamin-Roman Lyamin-Roman left a comment

Choose a reason for hiding this comment

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

LGTM for GPU part

do { \
auto _ov_check_condition = [](const auto& v) -> void { \
using _ov_check_t = ::std::remove_reference_t<decltype(v)>; \
if constexpr (::std::is_array_v<_ov_check_t> && \
Copy link
Contributor

@Lyamin-Roman Lyamin-Roman Feb 2, 2026

Choose a reason for hiding this comment

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

Does it make sense to have different error messages for different types of strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, removed

@aobolensk aobolensk force-pushed the core-string-lit-assert branch from c649f8f to 5d630a6 Compare February 2, 2026 13:02
Copy link
Contributor

@praasz praasz left a comment

Choose a reason for hiding this comment

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

Check binary size

@aobolensk aobolensk force-pushed the core-string-lit-assert branch from 0e431b4 to 1fa16fa Compare February 20, 2026 09:53
explicit NotImplemented(const std::string& what_arg) : ov::AssertFailure(what_arg) {}
};

namespace detail {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this section and hide inside the macro. There should be more public functions to handle OV exceptions for internal checks

auto dest_type_i64 = std::pair<std::string, detail::AttrAny>({"destination_type", "i64"});
auto dest_type_f32 = std::pair<std::string, detail::AttrAny>({"destination_type", "f32"});
auto dest_type_f16 = std::pair<std::string, detail::AttrAny>({"destination_type", "f16"});
auto dest_type_i64 = std::pair<std::string, gen_pattern::detail::AttrAny>({"destination_type", "i64"});
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be required because of exception changes.

@aobolensk aobolensk changed the title Add compile time guard to prevent string literal conditions in OPENVINO_ASSERT Fix string literal conditions used in OPENVINO_ASSERT Feb 27, 2026
@aobolensk
Copy link
Contributor Author

The solution is cumbersome if we use C++17 only. There is no robust way to implement in a proper way without triggering many other compiler warnings. Decided to postpone proper solution until C++20 enabling. For now, fixed all incorrect assertions only without changes to OPENVINO_ASSERT. ping @v-Golubev

@aobolensk aobolensk requested a review from praasz February 27, 2026 09:06
@github-actions github-actions bot removed category: Core OpenVINO Core (aka ngraph) category: transformations OpenVINO Runtime library - Transformations category: CPP API OpenVINO CPP API bindings labels Feb 27, 2026
Copy link
Contributor

@v-Golubev v-Golubev left a comment

Choose a reason for hiding this comment

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

LGTM from CPU side

@sys-openvino-ci sys-openvino-ci added the ExternalPR External contributor label Mar 19, 2026
@praasz
Copy link
Contributor

praasz commented Mar 20, 2026

build_jenkins

@praasz praasz added this to the 2026.2 milestone Mar 20, 2026
@v-Golubev
Copy link
Contributor

build_jenkins

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

Labels

category: CPU OpenVINO CPU plugin category: GPU OpenVINO GPU plugin category: HETERO OpenVINO HETERO plugin category: LP transformations OpenVINO Low Precision transformations ExternalPR External contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants