WIP: Logger Strawman #2475

jzulauf-lunarg · 2018-03-09T22:45:22Z

TODO: Rebase this on top of MikeS recent changes. (ignore the push constant changes... rebase conflicts were messy, so I left it on the branch I started with)

For your review/critique is a quick and dirty working prototype of a new message logger for the validation layers.

The goals were:

No repetition of known data (i.e. types/VUIDs)
No casts when constructing messages.
Flexible formatting options.
No formatting if message will not be emitted.
A starting place for more brainstorming

I've given 3 examples of what the new message create would look like. The programming flow is:

Create logger object with the fixed data
Add to object (noop'd if flags are not enabled)
- sprintf style
- add* methods (including handle and pointer automated handling)
- << for types where built-in stream out is acceptable
Either manually post the the report (if no skip variable is present) --or--
Allow the logger object to go out of scope ("report" is called in the destructor if not done already)

Missing:

even better automatic casting/overload. (given further code generation)
detailed interface design (naming, and method vs. operator)

Messy:

Use of MACRO for msg generation (to compress flags and LINE)
add_x and add_h for hex and handle handling...
transitions between built-in type support (<< operations) and .add* method operations
Using << style formatting. (maybe not familiar to maintainers)

Pretty Cool:

Automatic object type detection
No-op output functions if messages are not enabled
Built-in sprintf conversions
Automatic update of "skip", and post to debug_log_msg in report/destructor
Automatic appending of validation_error_message (when avail)
No more PRI ...

Updated the core validation layer and matching units to reflect changes to the valid usage statements in the Vulkan specification to better support the use of overlapping push constant ranges. The following valid usage was removed from the specification and the core validation layer. VUID-vkCmdPushConstants-stageFlags-00367 VALIDATION_ERROR_1bc002de Two new valid usages have been added to the core validation layer. VALIDATION_ERROR_1bc00e06 VUID-vkCmdPushConstants-offset-01795 VALIDATION_ERROR_1bc00e08 VUID-vkCmdPushConstants-offset-01796 The individual unit tests within the larger InvalidPushConstants test case have been updated to reflect the changes to the valid usage checks. Change-Id: I9bba3efa19f96c52e82b328bee64f8f2fbb10342

TODO: Rebase this on top of MikeS recent changes. For your review/critique is a quick and dirty working protoptype of a new message logger for the validation layers. The goals were: * No repetition of known data (i.e. types/VUIDs) * No casts when constructing messages. * Flexible formatting options. * No formatting if message will not be emitted. * A starting place for more brainstorming * Quick and dirty Missing: * even better automatic casting/overload. (given further code generation) * detailed interface design (naming, and method vs. operator) Messy: * Use of MACRO for msg generation (to compress flags and __LINE__) * add_x and add_h for hex and handle handling... * transitions between built-in type support (<< operations) and .add* method operations * Using << style formatting. (maybe not familiar to maintainers) Pretty Cool: * Automatic object type detection * No-op output functions if messages are not enabled * Built-in sprintf conversions * Automatic update of "skip", and post to debug_log_msg in destructor * Automatic appending of validation_error_message (when avail) * No more PRI ... Change-Id: I5ea8f5e95c9d765ab846038d6ceab60fc111d3d4

jzulauf-lunarg

Some implementation notes.

jzulauf-lunarg · 2018-03-09T22:56:43Z

layers/vk_layer_logging.h

 #ifndef WIN32
-static inline int string_sprintf(std::string *output, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
+static inline std::string string_sprintf(std::string *to_string, const char *fmt, ...) __attribute__((format(printf, 2, 3)));


ignore this... I was planning to use it in the logger and discovered it wasn't actually working... these changes to show up in a separate PR.

jzulauf-lunarg · 2018-03-09T22:57:43Z

layers/vk_layer_logging.h

@@ -439,4 +455,131 @@ uint64_t HandleToUint64(HANDLE_T h);

 static inline uint64_t HandleToUint64(uint64_t h) { return h; }

+class LogMsg {


Clearly some comments are in order here... see the PR description for the "theory of operation" stuff.

jzulauf-lunarg · 2018-03-09T22:58:30Z

layers/vk_layer_logging.h

+        return *this;
+    }
+
+        // use sprintf to add formatted information


Thar be va_dragons here. See something, say something.

jzulauf-lunarg · 2018-03-09T22:59:31Z

layers/vk_layer_logging.h

+    }
+
+    bool will_report() const { return will_report_; };
+    bool report() {


This call is only needed if you supply a nullptr status... otherwise, just going out of scope takes care of sending the debug_report_log_msg

jzulauf-lunarg · 2018-03-09T23:00:56Z

layers/vk_layer_logging.h

+    return logger.add(out);
+}
+
+#define LOG_ERR_MSG(skip, msg, report_data, object, code, prefix) \


if the __LINE__ and prefix are going away, we can avoid the macro by subclassing LogMsg for the two common cases without loss of brevity.

jzulauf-lunarg · 2018-03-09T23:02:50Z

scripts/helper_file_generator.py

-                if search_type == target_type:
-                    object_types_header += '    %s,   // %s\n' % (vk_object_type, object_type)
-                    break
+            vk_object_type = vko_map[object_type.replace("kVulkanObjectType", "").lower()]


since I already have the map, why not use it.

jzulauf-lunarg · 2018-03-09T23:04:07Z

scripts/helper_file_generator.py

+        for object_type in type_list:
+            ot_key = object_type.replace("kVulkanObjectType", "").lower()
+            vk_type = object_type.replace("kVulkanObjectType", "Vk")
+            object_types_header += '// Object type enums for %s\n' % (vk_type)


You could get away with just the kVulkanObjectType enum (and look up the others) but as the compiler is likely just constexpr the whole thing, it's free so have them all.

jzulauf-lunarg · 2018-03-09T23:04:47Z

layers/vk_layer_logging.h

+
+    template <typename IntType>
+    LogMsg &add_x(IntType out) {
+        if (will_report_) {


note the noop'ing controlled by will_report_

jzulauf-lunarg · 2018-03-09T23:06:37Z

layers/core_validation.cpp

-                            HandleToUint64(pCB->commandBuffer), pPool->queueFamilyIndex, HandleToUint64(queue),
-                            queue_state->queueFamilyIndex, validation_error_map[VALIDATION_ERROR_31a00094]);
+            LOG_ERR_MSG(&skip, msg, dev_data->report_data, pCB->commandBuffer, VALIDATION_ERROR_31a00094, "DS");
+            msg.add("vkQueueSubmit: Primary command buffer ").add_h(pCB->commandBuffer);


with more code gen, the add_h could be removed in favor of "overloads per handle type", s.t. all handles are known for what they are.

skip, report_data, objType + object, layer name, and function name are usually constants for the whole function.
Would be nice not having to repeat them. Maybe class Logger? Then Logger log(all the above) and then per case log.err(veid, msg) or log.warn()?

Perhaps a LoggerContext that could be used in a LogMsg constructor

jzulauf-lunarg · 2018-03-09T23:10:40Z

layers/core_validation.cpp

-                           "%s: Cannot read invalid region of memory allocation 0x%" PRIx64 " for bound %s object 0x%" PRIx64
-                           ", please fill the memory before using.",
-                           functionName, HandleToUint64(mem), object_string[type], bound_object_handle);
+            LOG_WARN_MSG(nullptr, msg, dev_data->report_data, mem, MEMTRACK_INVALID_MEM_REGION, "MEM");


In looking for good examples I stumbled across VerifyBoundMemoryIsValid , which is using the wrong object types (hard coded image for things that can be buffers)... implementing the auto detection is going to take changing some interfaces (like that one) to not convert handles (which will probably fix some latent bugs) and templatizing things like VerifyBoundMemoryIsValid... that or adding LogMsg constructors that take uint64_t/object_type pairs.

mark-lunarg · 2018-05-11T17:04:34Z

@jzulauf-lunarg, this repository will close for write access on Sunday, 5/13/2018. If it is pushed before that time it will be present in the follow-on Vulkan-ValidationLayers repository on Monday, otherwise a new PR will be required in the new repo.

jzulauf-lunarg and others added 2 commits March 7, 2018 13:57

jzulauf-lunarg requested review from mark-lunarg, tobine, mikes-lunarg, daveh-lunarg and jeremyk-lunarg March 9, 2018 22:45

jzulauf-lunarg commented Mar 9, 2018

View reviewed changes

mark-lunarg mentioned this pull request Apr 20, 2018

Add a way to name objects for validation messages #1880

Open

Jasper-Bekkers mentioned this pull request Apr 8, 2020

Names for objects in validation layer, take 2 KhronosGroup/Vulkan-ValidationLayers#1695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Logger Strawman #2475

WIP: Logger Strawman #2475

jzulauf-lunarg commented Mar 9, 2018 •

edited

Loading

jzulauf-lunarg left a comment

jzulauf-lunarg Mar 9, 2018

jzulauf-lunarg Mar 9, 2018

jzulauf-lunarg Mar 9, 2018

jzulauf-lunarg Mar 9, 2018

jzulauf-lunarg Mar 9, 2018

jzulauf-lunarg Mar 9, 2018

jzulauf-lunarg Mar 9, 2018

jzulauf-lunarg Mar 9, 2018

jzulauf-lunarg Mar 9, 2018

krOoze Mar 10, 2018

jzulauf-lunarg Mar 11, 2018

jzulauf-lunarg Mar 9, 2018

mark-lunarg commented May 11, 2018

		@@ -439,4 +455,131 @@ uint64_t HandleToUint64(HANDLE_T h);

		static inline uint64_t HandleToUint64(uint64_t h) { return h; }

		class LogMsg {

WIP: Logger Strawman #2475

Are you sure you want to change the base?

WIP: Logger Strawman #2475

Conversation

jzulauf-lunarg commented Mar 9, 2018 • edited Loading

jzulauf-lunarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-lunarg commented May 11, 2018

jzulauf-lunarg commented Mar 9, 2018 •

edited

Loading