[Mesa-dev] [PATCH 06/15] clover/spirv: Add functions for validating SPIR-V binaries

[Pierre Moreau]

> > +      auto const level_to_string = [](spv_message_level_t level){
> 
> "const auto" would be more idiosyncratic.
> 
> > +         switch (level) {
> > +            case SPV_MSG_FATAL:
> > +               return std::string("Fatal");
> > +            case SPV_MSG_INTERNAL_ERROR:
> > +               return std::string("Internal error");
> > +            case SPV_MSG_ERROR:
> > +               return std::string("Error");
> > +            case SPV_MSG_WARNING:
> > +               return std::string("Warning");
> > +            case SPV_MSG_INFO:
> > +               return std::string("Info");
> > +            case SPV_MSG_DEBUG:
> > +               return std::string("Debug");
> > +         }
> > +         return std::string();
> > +      };
> 
> You could just use a 'level == X ? "Y" : ...' ladder here instead since
> the 'level' argument is already known at the point the level_to_string
> function is defined.

Karol took the switch out of the lambda function and removed the lambda; I
can’t remember why I had it that.

> > +bool
> > +clover::spirv::is_binary_spirv(const char *il, size_t length)
> > +{
> > +   const uint32_t *binary = reinterpret_cast<const uint32_t*>(il);
> > +
> > +   // A SPIR-V binary is at the very least 5 32-bit words, which represent the
> > +   // SPIR-V header.
> > +   if (length < 20u)
> > +      return false;
> > +
> > +   const uint32_t first_word = binary[0u];
> > +   return (first_word == SpvMagicNumber) ||
> > +          (util_bswap32(first_word) == SpvMagicNumber);
> > +}
> > +
> 
> This function seems like dead code.  Maybe drop it?  Then you'll be able
> to use a single #ifdef preprocessor conditional to guard the whole file.

The code using it was moved to a separate MR which will be sent after this one;
the function has now been moved to the later MR.

> 
> > +#ifdef CLOVER_ALLOW_SPIRV
> > +bool
> > +clover::spirv::is_valid_spirv(const uint32_t *binary, size_t length,
> > +                              const std::string &opencl_version,
> > +                              const context::notify_action &notify) {
> 
> We don't need an extra level of call-backs here, you can pass a
> 'std::string &r_log' reference and append the errors there, just like
> the LLVM codepaths do too return error strings.

Would this work better for you?
```
bool
clover::spirv::is_valid_spirv(const uint32_t *binary, size_t length,
                              const std::string &opencl_version,
                              std::string &r_log) {
   auto const validator_consumer =
      [&notify](spv_message_level_t level, const char *source,
                const spv_position_t &position, const char *message) {
      r_log += format_validator_msg(level, source, position,
                                                   message);
   };

   const spv_target_env target_env =
      convert_opencl_str_to_target_env(opencl_version);
   spvtools::SpirvTools spvTool(target_env);
   spvTool.SetMessageConsumer(validator_consumer);

   return spvTool.Validate(binary, length);
}
```
And then it would be the caller’s responsibility to get the string back to the
user.

> Other than that looks good:
> 
> Reviewed-by: Francisco Jerez <currojerez at riseup.net>

Thank you for the review!

Pierre
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190512/eb2eaed2/attachment.sig>