Cover remaining tinyformat usages in CheckFormatSpecifiers #30999

pull l0rinc wants to merge 5 commits into bitcoin:master from l0rinc:l0rinc/ConstevalFOrmatString changing 2 files +123 −103
  1. l0rinc commented at 6:54 pm on September 29, 2024: contributor

    Split out of #30928 (review)

    The current string formatter couldn’t validate every string format template that we were using. Extended it with dynamic widths, fixed a number parsing bug that could go over the string’s content and added a %n validation.

  2. DrahtBot commented at 6:54 pm on September 29, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Tests on Sep 29, 2024
  4. l0rinc renamed this:
    test: streamline CheckFormatSpecifiers testability
    Cover remaining tinyformat usages in CheckFormatSpecifiers
    on Sep 29, 2024
  5. l0rinc marked this as ready for review on Sep 29, 2024
  6. in src/test/util_string_tests.cpp:17 in cba51f2e38 outdated
    10@@ -11,75 +11,61 @@ using namespace util;
    11 
    12 BOOST_AUTO_TEST_SUITE(util_string_tests)
    13 
    14-// Helper to allow compile-time sanity checks while providing the number of
    15-// args directly. Normally PassFmt<sizeof...(Args)> would be used.
    16-template <unsigned NumArgs>
    17-inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
    


    maflcko commented at 6:35 am on September 30, 2024:

    in cba51f2e380f0f89f799369b489c2e25e7215221: Not sure about turning the passing one into a runtime check from a compile-time check. Previously it was trivial to compile a single unit (this test) to sanity check the parser, as well as check the compile failures for various errors, by simply looking at the compile output. Also, the code was as close as possible to the real code, serving as documentation of how to use it.

    Now, checking the passing cases requires not only compiling, but also linking and executing the test. Also, triggering a compile error to see that it works and to see how it looks is harder.

    I understand you want better error messages on failure, but changing the passing cases isn’t required for that.


    l0rinc commented at 11:05 am on September 30, 2024:
    Valid complaint, I’ve added a consteval ValidFormatSpecifiers local delegate and split the valid tests from the BOOST_CHECK_EXCEPTIONs - this way failures still show the correct lines both for the valid and invalid tests.

    maflcko commented at 11:54 am on September 30, 2024:

    What is the benefit of ValidFormatSpecifiers over the existing PassFmt, other than dropping the code coverage stats?

    Seems fine to update the comment below to say // Execute compile-time check again at run-time to get code coverage stats., but not sure about dropping it.


    l0rinc commented at 12:11 pm on September 30, 2024:
    I’ve added the comment back, that’s indeed important context. Compared to PassFmt I found the ValidFormatSpecifiers to be more specific (I’m not a fan of abbrvs and // comments). Don’t have strong preference here, I can be convinced to rename it back, if you do.

    maflcko commented at 7:34 am on October 1, 2024:

    Compared to PassFmt I found the ValidFormatSpecifiers to be more specific (I’m not a fan of abbrvs and // comments).

    I don’t care about naming, so if you want to rename PassFmt to something else, this is fine. However, the // comment isn’t useless: It explains that the goal of this helper function is to be close as possible to the real code (and document the only difference). I found that useful as a single compilation unit that serves as a close proxy to the real code, with almost the same compile-time error messages (and behavior).

    I understand you want better error messages on failure, but changing the behavior of the passing cases isn’t required for that.

    Generally, if there isn’t a reason to change something, it is better to leave the code as-is, because it was most likely intentionally written in that way.

  7. in src/util/string.h:77 in cba51f2e38 outdated
    113+    if (count_normal && count_pos) throw "Format specifiers must be all positional or all non-positional!";
    114+    int count{count_normal | count_pos};
    115+    if (num_params != count) throw "Format specifier count must match the argument count!";
    116+}
    117+
    118+template <int num_params>
    


    maflcko commented at 6:38 am on September 30, 2024:
    nit: not sure about changing this back again to accommodate test-only code. Instead of using -1 for test code, you could set constexpr INVALID=-1; and use that.

    l0rinc commented at 11:05 am on September 30, 2024:
    Good idea, thanks
  8. in src/util/string.h:33 in 23f2887af3 outdated
    24@@ -25,58 +25,58 @@ namespace util {
    25  * strings, to reduce the likelihood of tinyformat throwing exceptions at
    26  * run-time. Validation is partial to try and prevent the most common errors
    27  * while avoiding re-implementing the entire parsing logic.
    28- *
    29- * @note Counting of `*` dynamic width and precision fields (such as `%*c`,
    30- * `%2$*3$d`, `%.*f`) is not implemented to minimize code complexity as long as
    31- * they are not used in the codebase. Usage of these fields is not counted and
    32- * can lead to run-time exceptions. Code wanting to use the `*` specifier can
    33- * side-step this struct and call tinyformat directly.
    


    maflcko commented at 6:50 am on September 30, 2024:
    Why remove this comment? format("'%1$*3$s %2$-*3$s'", "hi", "w", 12) is still unsupported and parsed incorrectly at compile time.

    l0rinc commented at 11:05 am on September 30, 2024:
    Added back the part that I think is relevant, let me know if you’d like me to rewrite it.

    maflcko commented at 11:56 am on September 30, 2024:
    Those are dynamic width fields, so I still don’t understand why you remove that from the comment.

    l0rinc commented at 12:11 pm on September 30, 2024:
    Because we do have some dynamic width support for the values that were used in the codebase. But I’ve reverted the original comment (but deleted %*c which is a compile-time failure now).
  9. maflcko commented at 6:51 am on September 30, 2024: member
    d49dfafa9aea349ae6741f49ed992b78a88b6839 looks correct, but I am not sure the others.
  10. l0rinc force-pushed on Sep 30, 2024
  11. test: streamline CheckFormatSpecifiers testability
    * Renamed `Detail_CheckNumFormatSpecifiers` to `CheckFormatSpecifiers` since we're checking more than the number of parameters
    * Moved it out of `ConstevalFormatString` to make it easier to test
    * Inline `FailFmtWithError` (and rename `PassFmt` to `ValidFormatSpecifiers`) in tests to provide better errors messages on failure (e.g. line number)
    ab53e6560e
  12. test: Unify CheckFormatSpecifiers error messages f701cca3e1
  13. CheckFormatSpecifiers shouldn't iterate beyond string bounds 51c56e8b38
  14. Implement dynamic width validation in CheckFormatSpecifiers
    They were used in bitcoin-cli
    49fc242ad3
  15. Prohibit %n usages in format
    It's not supported in tinyformat: https://github.com/bitcoin/bitcoin/blob/master/src/tinyformat.h#L843-L845
    6e4935df88
  16. l0rinc force-pushed on Sep 30, 2024
  17. in src/util/string.h:31 in 6e4935df88
    25@@ -26,57 +26,63 @@ namespace util {
    26  * run-time. Validation is partial to try and prevent the most common errors
    27  * while avoiding re-implementing the entire parsing logic.
    28  *
    29- * @note Counting of `*` dynamic width and precision fields (such as `%*c`,
    30+ * @note Counting of `*` dynamic width and precision fields (such as
    31  * `%2$*3$d`, `%.*f`) is not implemented to minimize code complexity as long as
    32  * they are not used in the codebase. Usage of these fields is not counted and
    


    maflcko commented at 7:44 am on October 1, 2024:

    not sure about implementing a random and specific subset of * in specifiers. I think it is easier to either fully support them, or not at all. But having developers read the parser to understand which subset they are allowed to use may be causing more frustration than solving any real issue.


    l0rinc commented at 8:14 am on October 1, 2024:

    not implemented to minimize code complexity as long as they are not used in the codebase

    I’ve implemented the part that was used, not a “random subset”

  18. in src/util/string.h:35 in 6e4935df88
    31  * `%2$*3$d`, `%.*f`) is not implemented to minimize code complexity as long as
    32  * they are not used in the codebase. Usage of these fields is not counted and
    33  * can lead to run-time exceptions. Code wanting to use the `*` specifier can
    34  * side-step this struct and call tinyformat directly.
    35  */
    36+constexpr static void CheckFormatSpecifiers(std::string_view str, unsigned num_params)
    


    maflcko commented at 7:48 am on October 1, 2024:

    Not sure about moving this out. This will break the doxygen comment above. Also, it drops the “detail-namespace”.

    Generally, I think that test-only code should follow the real code, not the other way round. As long as real code is testable, optimizing other parts of the unit tests doesn’t seem too useful, especially if it breaks the existing construct and documentation.

  19. maflcko commented at 7:51 am on October 1, 2024: member
    As mentioned previously, it looks like there is one correct commit. However, I have a hard time seeing how the others are useful in a great picture, given that some of them are incomplete anyway.
  20. l0rinc commented at 8:15 am on October 1, 2024: contributor
    I’m closing it for lack of interest, feel free to cherry-pick changes to other PRs
  21. l0rinc closed this on Oct 1, 2024


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me