bryancall opened a new pull request, #13724:
URL: https://github.com/apache/trafficserver/pull/13724
## What
`Base_Binary_Cmp::TYPES` in
`plugins/experimental/txn_box/plugin/src/Comparison.cc` is the type
mask that gates `eq`, `ne`, `lt`, `le`, `gt` and `ge` in
`Comparison::load()`. It was initialized
from another static:
```cpp
static inline const ActiveType TYPES = Cmp_Types::apply<ValueMaskFor>::value;
```
`ValueMaskFor<F...>::value` is a static data member of a class template, so
its dynamic
initialization is unordered. `TYPES` is a plain inline variable, so its
initialization is
partially ordered. Nothing sequences the two, so `TYPES` can be built from a
mask that still holds
its zero-initialized value.
This uses the direct `ActiveType` constructor instead, the same form the
other comparisons in this
file use (for example `Cmp_in::TYPES{INTEGER, IP_ADDR}`), so `TYPES` no
longer reads another
variable:
```cpp
static inline const ActiveType TYPES{INTEGER, BOOLEAN, IP_ADDR, DURATION};
```
`ValueMaskFor` and the type-list `MaskFor<F...>()` it used had no other
callers and are removed.
## Who this affects
Measured by reading `Base_Binary_Cmp::TYPES` out of the real `txn_box.so`
after it is loaded,
built through the project's own CMake (so C++17, which txn_box pins). The
correct value is `0xd8`,
i.e. `INTEGER|BOOLEAN|IP_ADDR|DURATION`.
| toolchain | master | this branch |
| --- | --- | --- |
| GCC 15.2.1 (`ci.trafficserver.apache.org/ats/fedora:42`) | `0xd8` | `0xd8`
|
| clang 20.1.8 (same image) | **`0x0`** | `0xd8` |
| Apple clang 21 (macOS) | **`0x0`** | `0xd8` |
An empty mask fails `can_satisfy()` for every feature, so any config using
one of the six
comparators is rejected at load with "Comparison ... is not valid for active
feature".
In CI, `ci-fedora-cxx20`, `ci-ubuntu` and `ci-clang-analyzer` build txn_box
with clang, as do
`ci-osx` and `ci-freebsd` with the platform compiler. The autest lanes build
with GCC. So CI builds
the affected plugin but never exercises it, which is why the txn_box autests
are green. Anyone
building on macOS, for example with the `release` preset, gets the empty
mask.
## Tests
`tests/gold_tests/pluginTest/txn_box/basic/cmp.replay.yaml` is the dedicated
comparison autest, and
it only covered the string comparators. It now also checks `eq`, `ne`, `lt`,
`le`, `gt` and `ge`
against an integer feature, with the four transactions carrying 30, 29, 31
and 10 around a boundary
of 30.
That adds coverage of the comparators themselves. For this particular bug it
adds no detection the
suite lacked, since `txn_box_ip-addr` already uses `eq` on an IP address and
`txn_box_ramp` uses `lt`
on an integer. None of these can fail on the GCC autest lane, where the mask
is correct.
On clang they do: in the CI image, `txn_box_cmp` and `txn_box_ip-addr` fail
against master with
"not valid for active feature" in the logs, and pass on this branch.
## Verification
- Built in `ci.trafficserver.apache.org/ats/fedora:42` with GCC 15.2.1 and
with clang 20.1.8, with `BUILD_EXPERIMENTAL_PLUGINS=ON`. Both full builds are
clean.
- `test_txn_box` passes.
- GCC, all 39 txn_box autests: 35 pass and 3 are skipped by their own test
files. `txn_box_ramp` fails in that container only because autests ran as root
there, and `traffic_server` cannot read the test's `0600` temp config file
(`EACCES` at file open, before txn_box parses anything).
- clang, master: `txn_box_cmp` and `txn_box_ip-addr` fail. clang, this
branch: both pass.
`fedora:43` is currently unpullable from the CI registry, so `:42` was used.
## Not in this PR
The registration block passes `Cmp_le::TYPES` for `lt` and `Cmp_lt::TYPES`
for `le`. It is harmless,
since both inherit `Base_Binary_Cmp::TYPES`, but it reads like a bug and
deserves its own one-line
change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
