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.

Sign up for GitHub

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

Jump to bottom

NoNewGlobals for HttpHdrCc:ccLookupTable #1750

Closed
wants to merge 14 commits into from
Closed

NoNewGlobals for HttpHdrCc:ccLookupTable #1750

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1f3b01f
Convert CcAttrs to lazy initialization
kinkie Mar 19, 2024
0909b48
Convert ccLookupTable
kinkie Mar 19, 2024
a82a346
Move invariant checking to CcAttrs()
kinkie Mar 19, 2024
0304a93
Remove httpHdrCcInitModule
kinkie Mar 19, 2024
f3d3cc0
Implement ccTypeByName
kinkie Mar 19, 2024
1737f0a
Implement ccNameByType
kinkie Mar 19, 2024
1f283fa
Fully rely on ccNameByType
kinkie Mar 19, 2024
9a34d90
aaa
kinkie Mar 19, 2024
076f973
fixup: Use compile-time checks for attrsList
rousskov Mar 20, 2024
692a947
fixup: Do not assume HttpHdrCcType os int-compatible (pedantic)
rousskov Mar 20, 2024
e48c4c7
fixup: Fix and encapsulate tricky number-to-ID conversion code
rousskov Mar 20, 2024
e061015
fixup: Minor polishing
rousskov Mar 20, 2024
88690dc
Apply suggestions from code review
kinkie Mar 21, 2024
161cce4
Merge branch 'master' into coverity-cid-1554655
kinkie Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 49 additions & 40 deletions src/HttpHdrCc.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,50 +9,54 @@
/* DEBUG: section 65 HTTP Cache Control Header */

#include "squid.h"
#include "base/EnumIterator.h"
#include "base/LookupTable.h"
#include "HttpHdrCc.h"
#include "HttpHeader.h"
#include "HttpHeaderFieldStat.h"
#include "HttpHeaderStat.h"
#include "HttpHeaderTools.h"
#include "sbuf/SBuf.h"
#include "SquidMath.h"
#include "StatHist.h"
#include "Store.h"
#include "StrList.h"
#include "util.h"

#include <map>
#include <vector>
#include <optional>
#include <ostream>

static const auto&
constexpr LookupTable<HttpHdrCcType>::Record attrsList[] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT, this compile-time constant is no longer a "global" in "No New Globals" sense. It cannot be localized inside CcAttrs() AFAICT because the compiler does not like us returning a reference to a local constexpr. Making this constexpr allows us to check the invariant at compile time, which is a significant improvement. Long-term, I suspect that this initialization-only code will be significantly refactored to avoid nil names and CC_ENUM_END hacks. However, if constexpr is not enough to make Coverity or others happy, we can go back to local statics and runtime assertions. I do not insist on trying constexpr here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree to try it. It might become a useful pattern, as you say.
I'm not sure we should strive to make Coverity happy; either we are solving real (if small) problems or we should just tell Coverity to be quiet

Copy link
Contributor

Choose a reason for hiding this comment

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

Returning a reference to a variable which exists only in compiler memory makes no sense. But leaving it a static member like all our other NoNewGlobals functions have should be fine to return and also more compatible with future conversion of the array to storing SBuf

Copy link
Contributor

Choose a reason for hiding this comment

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

TLDR: @yadij, if you think that the current PR code does not resolve these and other tradeoffs well and should go back to runtime checks, please say so explicitly, and I will adjust the code accordingly. I do not think we should merge this PR unless there is consensus regarding these compile-time checks. If you do not think these compile-time checks are worth their associated risks and limitations, we should not merge.

Returning a reference to a variable which exists only in compiler memory makes no sense.

Why not?! IMHO, it may make sense to return a reference to a variable used by the compiler.

Moreover, constexpr does not imply that the variable exists only in compiler memory: The specifier means that the value of that variable can be computed at compile time. The variable existence duration is governed by other considerations. For example, in the current PR code, attrsList exists at runtime in my tests:

$ nm src/squid | c++filt | grep attrsList
0000000000b241a0 d attrsList

(gdb) p attrsList
$1 = {{name = 0x555555e990a8 "public", id = CC_PUBLIC}, ...}

If we were to add extern ... attrList[] declaration, then attrList would become visible in other translation units as well (while still being constexpr).

leaving it a static member ... should be fine to return

Yes, but, as I tried to explain, doing so will kill compile-time checks because static variables are prohibited inside constexpr functions (until C++23):

HttpHdrCc.cc:34:53: error: ‘attrsList’ declared ‘static’ in ‘constexpr’ function

and also more compatible with future conversion of the array to storing SBuf

I am not sure we would want to convert this particular array, but if we do start storing SBuf in it, then we would not be able to check its invariants at compile time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know whether ASAN correctly detects the limits of a loop scanning across the constexpr when only that virtual-reference is provided (ie before the expression is calculated?).

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know whether ASAN correctly detects the limits of a loop scanning across the constexpr when only that virtual-reference is provided (ie before the expression is calculated?).

I assume that by "ASAN" you mean "address sanitizer" (more commonly abbreviated as ASan AFAICT).

I do not know much about address sanitizers, but I would expect their limitations to depend on the address sanitizer implementation. I can speculate that if a sanitizer is reusing initial processing stages from the "compiler", then it may not even have to deal with constexpr computations -- it would rely on their computed values received from those earlier processing stages.

I also cannot guess what "virtual-reference" means in this context.

{"public", HttpHdrCcType::CC_PUBLIC},
{"private", HttpHdrCcType::CC_PRIVATE},
{"no-cache", HttpHdrCcType::CC_NO_CACHE},
{"no-store", HttpHdrCcType::CC_NO_STORE},
{"no-transform", HttpHdrCcType::CC_NO_TRANSFORM},
{"must-revalidate", HttpHdrCcType::CC_MUST_REVALIDATE},
{"proxy-revalidate", HttpHdrCcType::CC_PROXY_REVALIDATE},
{"max-age", HttpHdrCcType::CC_MAX_AGE},
{"s-maxage", HttpHdrCcType::CC_S_MAXAGE},
{"max-stale", HttpHdrCcType::CC_MAX_STALE},
{"min-fresh", HttpHdrCcType::CC_MIN_FRESH},
{"only-if-cached", HttpHdrCcType::CC_ONLY_IF_CACHED},
{"stale-if-error", HttpHdrCcType::CC_STALE_IF_ERROR},
{"immutable", HttpHdrCcType::CC_IMMUTABLE},
{"Other,", HttpHdrCcType::CC_OTHER}, /* ',' will protect from matches */
{nullptr, HttpHdrCcType::CC_ENUM_END}
};

constexpr const auto &
CcAttrs() {
// invariant: row[j].id == j
static LookupTable<HttpHdrCcType>::Record attrsList[] = {
{"public", HttpHdrCcType::CC_PUBLIC},
{"private", HttpHdrCcType::CC_PRIVATE},
{"no-cache", HttpHdrCcType::CC_NO_CACHE},
{"no-store", HttpHdrCcType::CC_NO_STORE},
{"no-transform", HttpHdrCcType::CC_NO_TRANSFORM},
{"must-revalidate", HttpHdrCcType::CC_MUST_REVALIDATE},
{"proxy-revalidate", HttpHdrCcType::CC_PROXY_REVALIDATE},
{"max-age", HttpHdrCcType::CC_MAX_AGE},
{"s-maxage", HttpHdrCcType::CC_S_MAXAGE},
{"max-stale", HttpHdrCcType::CC_MAX_STALE},
{"min-fresh", HttpHdrCcType::CC_MIN_FRESH},
{"only-if-cached", HttpHdrCcType::CC_ONLY_IF_CACHED},
{"stale-if-error", HttpHdrCcType::CC_STALE_IF_ERROR},
{"immutable", HttpHdrCcType::CC_IMMUTABLE},
{"Other,", HttpHdrCcType::CC_OTHER}, /* ',' will protect from matches */
{nullptr, HttpHdrCcType::CC_ENUM_END}
};
static auto invariantChecked = false;
if (!invariantChecked) {
for (unsigned int j = 0; attrsList[j].name != nullptr; ++j) {
assert(static_cast<decltype(j)>(attrsList[j].id) == j);
}
invariantChecked = true;
}
// TODO: Move these compile-time checks into LookupTable
ConstexprForEnum<HttpHdrCcType::CC_PUBLIC, HttpHdrCcType::CC_ENUM_END>([](const auto ev) {
const auto idx = static_cast<std::underlying_type<HttpHdrCcType>::type>(ev);
// invariant: each row has a name except the last one
static_assert(!attrsList[idx].name == (ev == HttpHdrCcType::CC_ENUM_END));
// invariant: row[idx].id == idx
static_assert(attrsList[idx].id == ev);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to share how this static_assertion failure looks like (after injecting an incorrect attrsList entry):

HttpHdrCc.cc: In instantiation of ‘CcAttrs()::<lambda(auto:1)> [with auto:1 = std::integral_constant<HttpHdrCcType, CC_PUBLIC>]’:
HttpHdrCc.cc:62:39: error: static assertion failed
   62 |         static_assert(attrsList[j].id == j);

IMO, this is fairly readable and more informative than a runtime assertion would be (without using a debugger).

I wish I had tested the error message after injecting a bad value at the end of the table!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did. Entries past HttpHdrCcType::CC_ENUM_END are ignored

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant a bad entry before the end marker. I am worried about error message readability due to deep recursion...

We can add more static assertions to check for the presence of "extra" entries after the end marker as well, but, unlike duplicate or out-of-order entries before the end marker, such bugs would be pretty visible in the diff, so I am not particularly worried about them.

Copy link
Contributor Author

@kinkie kinkie Mar 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

On MacOS (clang):

/Users/kinkie/src/squid/coverity-cid-1554655/src/base/EnumIterator.h:237:5: note: in instantiation of function template specialization 'CcAttrs()::(anonymous class)::operator()<std::integral_constant<HttpHdrCcType, CC_ENUM_END>>' requestediff --git a/src/HttpHdrCc.cc b/src/HttpHdrCc.cc
index 2bd9303208..179be7de24 100644
--- a/src/HttpHdrCc.cc
+++ b/src/HttpHdrCc.cc
@@ -44,8 +44,8 @@ constexpr LookupTable<HttpHdrCcType>::Record attrsList[] = {
     {"stale-if-error", HttpHdrCcType::CC_STALE_IF_ERROR},
     {"immutable", HttpHdrCcType::CC_IMMUTABLE},
     {"Other,", HttpHdrCcType::CC_OTHER}, /* ',' will protect from matches */
-    {nullptr, HttpHdrCcType::CC_ENUM_END}
-};
+    {nullptr, HttpHdrCcType::CC_OTHER},
+    {nullptr, HttpHdrCcType::CC_ENUM_END}};

 constexpr const auto &
 CcAttrs() {

results in

/Users/kinkie/src/squid/coverity-cid-1554655/src/HttpHdrCc.cc:58:9: error: static assertion failed due to requirement 'attrsList[idx].id == ev'
        static_assert(attrsList[idx].id == ev);
        ^             ~~~~~~~~~~~~~~~~~~~~~~~
/Users/kinkie/src/squid/coverity-cid-1554655/src/base/EnumIterator.h:237:5: note: in instantiation of function template specialization 'CcAttrs()::(anonymous class)::operator()<std::integral_constant<HttpHdrCcType, CC_ENUM_END>>' requested here
    f(std::integral_constant<enumType, First>()); // including when First is Last
    ^
/Users/kinkie/src/squid/coverity-cid-1554655/src/base/EnumIterator.h:239:9: note: in instantiation of function template specialization 'ConstexprForEnum<CC_ENUM_END, CC_ENUM_END, (lambda at /Users/kinkie/src/squid/coverity-cid-1554655/src/HttpHdrCc.cc:53:76) &>' requested here
        ConstexprForEnum<enumType(static_cast<underlyingType>(First) + 1), Last>(f);
        ^
/Users/kinkie/src/squid/coverity-cid-1554655/src/base/EnumIterator.h:239:9: note: in instantiation of function template specialization 'ConstexprForEnum<CC_OTHER, CC_ENUM_END, (lambda at /Users/kinkie/src/squid/coverity-cid-1554655/src/HttpHdrCc.cc:53:76) &>' requested here
/Users/kinkie/src/squid/coverity-cid-1554655/src/base/EnumIterator.h:239:9: note: in instantiation of function template specialization 'ConstexprForEnum<CC_IMMUTABLE, CC_ENUM_END, (lambda at /Users/kinkie/src/squid/coverity-cid-1554655/src/HttpHdrCc.cc:53:76) &>' requested here
/Users/kinkie/src/squid/coverity-cid-1554655/src/base/EnumIterator.h:239:9: note: in instantiation of function template specialization 'ConstexprForEnum<CC_STALE_IF_ERROR, CC_ENUM_END, (lambda at /Users/kinkie/src/squid/coverity-cid-1554655/src/HttpHdrCc.cc:53:76) &>' requested here
/Users/kinkie/src/squid/coverity-cid-1554655/src/base/EnumIterator.h:239:9: note: (skipping 7 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
/Users/kinkie/src/squid/coverity-cid-1554655/src/base/EnumIterator.h:239:9: note: in instantiation of function template specialization 'ConstexprForEnum<CC_NO_TRANSFORM, CC_ENUM_END, (lambda at /Users/kinkie/src/squid/coverity-cid-1554655/src/HttpHdrCc.cc:53:76) &>' requested here
/Users/kinkie/src/squid/coverity-cid-1554655/src/base/EnumIterator.h:239:9: note: in instantiation of function template specialization 'ConstexprForEnum<CC_NO_STORE, CC_ENUM_END, (lambda at /Users/kinkie/src/squid/coverity-cid-1554655/src/HttpHdrCc.cc:53:76) &>' requested here
/Users/kinkie/src/squid/coverity-cid-1554655/src/base/EnumIterator.h:239:9: note: in instantiation of function template specialization 'ConstexprForEnum<CC_NO_CACHE, CC_ENUM_END, (lambda at /Users/kinkie/src/squid/coverity-cid-1554655/src/HttpHdrCc.cc:53:76) &>' requested here
/Users/kinkie/src/squid/coverity-cid-1554655/src/base/EnumIterator.h:239:9: note: in instantiation of function template specialization 'ConstexprForEnum<CC_PRIVATE, CC_ENUM_END, (lambda at /Users/kinkie/src/squid/coverity-cid-1554655/src/HttpHdrCc.cc:53:76) &>' requested here
/Users/kinkie/src/squid/coverity-cid-1554655/src/HttpHdrCc.cc:53:5: note: in instantiation of function template specialization 'ConstexprForEnum<CC_PUBLIC, CC_ENUM_END, (lambda at /Users/kinkie/src/squid/coverity-cid-1554655/src/HttpHdrCc.cc:53:76)>' requested here
    ConstexprForEnum<HttpHdrCcType::CC_PUBLIC, HttpHdrCcType::CC_ENUM_END>([](const auto ev) {
    ^
/Users/kinkie/src/squid/coverity-cid-1554655/src/HttpHdrCc.cc:58:41: note: expression evaluates to '14 == 15'
        static_assert(attrsList[idx].id == ev);
                      ~~~~~~~~~~~~~~~~~~^~~~~
1 error generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is exactly the test I was thinking about, thank you!

As I feared, it is not trivial to discover the enum value that violated the static assertion. The 14 == 15 error detail is probably enough to (eventually) figure out what is going. IMHO, this is not much worse than running a debugger after a runtime assertion (or even adding more code to report values that would lead to the assertion).

Overall, I still think that this static assertion experiment is worth merging, but I certainly do not insist on merging it.

});
return attrsList;
}

Expand All @@ -62,11 +66,18 @@ ccTypeByName(const SBuf &name) {
return table->lookup(name);
}

static auto
ccNameByType(HttpHdrCcType type) {
if (type < HttpHdrCcType::CC_ENUM_END)
return CcAttrs()[static_cast<int>(type)].name;
return "*invalid hdrcc*";
/// Safely converts an integer into a Cache-Control directive name.
/// \returns std::nullopt if the given integer is not a valid index of a named attrsList entry
template <typename RawId>
static std::optional<const char *>
ccNameByType(const RawId rawId)
{
// TODO: Store a by-ID index in (and move this functionality into) LookupTable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Other LookupTable code can benefit from this PR changes. Moreover, I suspect that many LookupTable users can be significantly improved by addressing this TODO and modernizing that code. However, it is probably best to do that refactoring in a dedicated PR rather than this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree.
This was exactly what I meant with my attempt for a reverse_lookup functionality

if (!Less(rawId, 0) && Less(rawId, int(HttpHdrCcType::CC_ENUM_END))) {
const auto idx = static_cast<std::underlying_type<HttpHdrCcType>::type>(rawId);
return CcAttrs()[idx].name;
}
return std::nullopt;
}

std::vector<HttpHeaderFieldStat> ccHeaderStats(HttpHdrCcType::CC_ENUM_END);
Expand Down Expand Up @@ -272,7 +283,7 @@ HttpHdrCc::packInto(Packable * p) const
if (isSet(flag) && flag != HttpHdrCcType::CC_OTHER) {

/* print option name for all options */
p->appendf((pcount ? ", %s": "%s"), ccNameByType(flag));
p->appendf((pcount ? ", %s": "%s"), *ccNameByType(flag));

/* for all options having values, "=value" after the name */
switch (flag) {
Expand Down Expand Up @@ -345,19 +356,17 @@ void
httpHdrCcStatDumper(StoreEntry * sentry, int, double val, double, int count)
{
extern const HttpHeaderStat *dump_stat; /* argh! */
const auto id = static_cast<HttpHdrCcType>(val);
const bool valid_id = id >= HttpHdrCcType::CC_PUBLIC && id < HttpHdrCcType::CC_ENUM_END;
auto name = valid_id ? ccNameByType(id) : "INVALID";

if (count || valid_id)
const int id = static_cast<int>(val);
const auto name = ccNameByType(id);
if (count || name)
storeAppendPrintf(sentry, "%2d\t %-20s\t %5d\t %6.2f\n",
id, name, count, xdiv(count, dump_stat->ccParsedCount));
id, name.value_or("INVALID"), count, xdiv(count, dump_stat->ccParsedCount));
yadij marked this conversation as resolved.
Show resolved Hide resolved
}

std::ostream &
operator<< (std::ostream &s, HttpHdrCcType c)
{
s << ccNameByType(c) << '[' << static_cast<int>(c) << ']';
s << ccNameByType(c).value_or("*invalid hdrcc*") << '[' << static_cast<int>(c) << ']';
kinkie marked this conversation as resolved.
Show resolved Hide resolved
return s;
}

15 changes: 15 additions & 0 deletions src/base/EnumIterator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,5 +224,20 @@ class WholeEnum : public EnumRangeT<EnumType>
WholeEnum() : EnumRangeT<EnumType>(EnumType::enumBegin_, EnumType::enumEnd_) {}
};

/// Compile-time iteration of sequential enum values from First to Last,
/// inclusive. The given function is called once on each iteration, with the
/// current enum value as an argument.
template <auto First, auto Last, class F>
constexpr void
ConstexprForEnum(F &&f)
{
using enumType = decltype(First);
using underlyingType = typename std::underlying_type<enumType>::type;
// std::integral_constant trick makes f(First) argument a constexpr
f(std::integral_constant<enumType, First>()); // including when First is Last
if constexpr (First < Last)
ConstexprForEnum<enumType(static_cast<underlyingType>(First) + 1), Last>(f);
}
yadij marked this conversation as resolved.
Show resolved Hide resolved

#endif /* SQUID_SRC_BASE_ENUMITERATOR_H */

Loading