Currently there is no testing for ClangTypeParser even though it's used in
production. This is because adding integration tests is very hard: they require
testing the build time behaviour at runtime, or else they'd be build failures
intead of test failures. There's a PR available for integration tests but it's
incomplete.
In contrast ClangTypeParser can be sort of unit tested. This follows the
structure of `test/test_drgn_parser.cpp` with some differences. There is a
tonne of boilerplate for setting up the Clang tool, and this set of testing
operates on type names instead of OID functions. The new tests are also
incredibly slow as they compile the entire `integration_test_target.cpp` (which
is huge) for every test case. I don't think this is avoidable without
compromising the separation of the tests somewhat due to the way Clang tooling
forces the code to be structured.
Currently I can't run these tests locally on a Meta devserver due to some
weirdness with the internal build and the `compile_commands.json` file. They
run in the CI and on any other open source machine though so I'm happy to merge
it - it's still useful. I'm going to close the PR to change the devserver build
given I'll be unable to follow up if it ends up being bad.
Test plan:
- CI
`std::variant` is the last archetypal container missing in TreeBuilder-v2. The
code for it isn't hugely complicated and relies on pack expansion.
This change introduces a new field to the container specification:
`scoped_extra`. This field allows you to write extra code that will be included
within the TypeHandler in CodeGen. This means it will not have collisions with
other containers, unlike the existing `extra` field. It's used here to write
the recursive `getSizeType` function for `std::variant`.
Tech debt is introduced here by comparing the container name to `std::variant`
in CodeGen to conditionally generate some code. We've worked hard to remove
references to containers in code and move them to `.toml` files. On balance,
this is worth having to include the example of `std::variant`. It should be
moved into a container spec field at some point, the design of which is still
to be determined.
Test plan:
- Activated the OIL `std::variant` tests.
- CI
Summary:
Correct identification of packing.
Tests were modified to reflect the new behaviour. One test was removed as it was bogus - the flattener pass runs before the alignmentcalc pass and therefore the layout in the test could never happen (i.e. it has a hierarchy).
Reviewed By: JakeHillion
Differential Revision: D53815661
CodeGen v2 permits template parameters to be qualified. This means that if we
call `make_field` with a template parameter it will be qualified. However, we
don't qualify the types when generating meta functions such as `NameProvider`
and `TypeHandler`. This means these qualified types don't match up with the
expected type.
Use `std::decay_t` when forwarding the type to `NameProvider` and `TypeHandler`
so they're always the base type that they were generated with. Most of this is
covered by `make_field`, but there are direct references to `TypeHandler<Ctx,
T>` in a lot of `TypeHandler::type` fields. Fix the problematic types manually
for now, there may need to be a better solution with meta functions for this in
the future.
Test Plan:
- CI
- Added a test for `std::unique_ptr<const uint64_t>` to exercise this. Failed
before, passes after.
- Added a test for `std::unique_ptr<const std::vector<uint64_t>>` to test a
non-primitive type. Failed before, passes after.
Summary:
OI was previously using `std::regex_match` to match container names. This was bad because `libstdc++`'s implementation of regex is awful. In the case of limited inlining it was causing a stack overflow when running CodeGen for large types (I think types with large names but I never got to the bottom of it).
Replace this with the competent `boost::regex_match` that we already have a dependency on.
Reviewed By: ajor
Differential Revision: D53002752
Thrift isset was failing with a SEGV if the struct contained padding. This is
because we indexed the `isset_indexes` data structure using our field index
rather than the index of the field in Thrift. This then gave a rubbish index
for any exceeding which happens if we have added padding in the middle of the
struct, and this index was looked up in the bitset which can cause a SEGV.
Track a new index `thriftFieldIdx` which is only incremented if we've looked up
a Thrift index.
Namespaced the generated Thrift structs while I was there. This isn't necessary
anymore but cleans things up.
Test plan:
- Added a test case with lots of padding. These don't run in the CI but it
passes locally.
- `FILTER='OilIntegration.*' make test` - no failures
- `FILTER='OidIntegration.*' make test` - no new failures
A previous change enabled running OIL tests with specific features enabled.
This highlighted that pointer code generation under TreeBuilder-v2 was very
broken. This change updates pointer code generation to work and enables the
skipped tests. All enabled tests need `expected_json_v2` added to them due to
formatting differences.
Reformatted and rewrote the basic type handler that handles primitives and
pointers. Removed the reliance on `features` to decide whether to generate for
TreeBuilder-v2 as the intermediate features have been removed. Pointers are
treated as containers with a capacity of 1 and a length of 0 if null/a cycle
and 1 if followed. This holds for void pointers where, although they aren't
followed, the length is still set.
There were a couple of other changes needed to enable these tests on TBv2 that
aren't worth their own issues and PRs, I sneaked them in here.
Extra changes:
- Added `Pointer` and `Reference` to TopoSorter so they generate
`NameProvider` instances. It might be worth visiting the graph differently
for `NameProvider` as it requires so many instances that others generators do
not. Will consider that in the future.
- Follow typedefs when calculating exclusive size for a type.
Closes#458.
Test plan:
- CI
- Enabled previously disabled tests.
Support the capture-thrift-isset feature with TreeBuilder-v2. Fairly minor
changes here except the type of the Enum in a template parameter now matters.
We follow the previous behaviour of capturing a value for each field in a
struct that has an `isset_bitset`. This value is a VarInt captured before the
C++ contents of the member. It has 3 values: 0 (not set), 1 (set), and 2
(unavailable). These are handled by the processor and represented in the output
as `false`, `true`, and `std::nullopt_t` respectively.
Changes:
- Add a simple Thrift isset processor before any fields that have Thrift isset.
- Store the fully qualified names of enum types in DrgnParser - it already
worked out this information anyway for naming the values and this is
consistent with classes.
- Forward all enum template parameters under their input name under the
assumption that they will all be policy type things like `IssetBitsetOption`.
This could turn out to be wrong.
Test plan:
- CI (doesn't test thrift changes but covers other regressions)
- Updated Thrift enum tests for new format.
- `FILTER='OilIntegration.*' make test` - Thrift tests failed before, succeed
after.
Previously we tested different feature flags by using the `cli_options` field
in the test `.toml`. This works for OID but doesn't work for JIT OIL and won't
work for AoT OIL when those tests get added.
This change adds a new higher level `features` field to the test `.toml` which
adds the features to the config file as a prefix. This works with all methods
of generation.
Change the existing `cli_options` features to `features` except for where
they're testing something specific. Enable tests that were previously disabled
for OIL but only didn't work because of not being able to enable features.
Change pointer tests that are currently broken for OIL from `oil_disable` to
`oil_skip` - they can work, but codegen is broken for them at the minute.
Test plan:
- CI
- `make test` is no worse
C++ has a concept of Primitive which holds in the type graph. However we don't
currently expose this information to the end user. Expose this from the OIL
iterator to allow future features like primitive rollups.
This affects containers like maps which have a fake `[]` element with no type.
They use this to group together the key/value in a map and to account for any
per element storage overhead. Currently the decision is to make the fake `[]`
element a primitive if all of its children are primitives. This allows for more
effective primitive rollups if that is implemented. This implementation detail
may be changed in future.
Test Plan:
- CI
- Updated simple tests.
Array members are currently being named "TODO" (whoops). Include arrays in
TopoSorter so each one can have a `NameProvider` generated in CodeGen. Then
pass array elements through `make_field`.
Test plan:
- CI
- Add array member names to an array test.
`std::list` has per element overhead for the individual heap allocations. This
was already calculated in the container implementation but not used. Allocate
the overhead of each element in the `std::list` to the `std::list` itself as
with other sequential containers.
Test Plan:
- CI
- Updated test cases
This test is a bit odd, but this change adds the full set of size/member checks
for the hierarchy for TreeBuilder v2 and enables it.
Closes#304
Test Plan:
- CI
Type names of optional elements were accidentally left as todo. Update
`std::optional` to use `make_field` and correctly name its elements.
Test Plan:
- CI
- Updated the integration tests to test the names.
Summary:
We have a good type representation in the Type Graph of an incomplete type and
the underlying type that represents. However, this incomplete type still ends
up in the generated code as `void` which loses information. For example, a
container that can't contain void may fail to compile because it was
initialised with `void` but really its because the type it was supposed to be
initialised with (say, `Foo`) had incomplete debug information.
This change identifies that a type is incomplete in the output by generating it
as an incomplete type `struct Incomplete<struct Foo>`. This allows us to name
the type correctly in the TreeBuilder output and filter for incomplete types,
as well as getting appropriate compiler errors if it mustn't be incomplete.
Test Plan:
- CI
- Added a unit test to namegen.
- Enabled and added an extra pointers_incomplete test.
This change is tricky to test because it isn't really user visible. The types
still use their `inputName` which is unchanged in any successful output - this
change is used so the compiler fails with a more detailed error.
Add the option to calculate total size (inclusive size) by wrapping the
existing iterator. This change provides a new iterator, `SizedIterator`, which
wraps an existing iterator and adds a new field `size` to the output element.
This is achieved with a two pass algorithm on the existing iterator:
1. Gather metadata for each element. This includes the total size up until that
element and the range of elements that should be included in the size.
2. Return the result from the underlying iterator with the additional
field.
This algorithm is `O(N)` time on the number of elements in the iterator and
`O(N)` time, storing 16 bytes per element. This isn't super expensive but is a
lot more than the current algorithm which requires close to constant space.
Because of this I've implemented it as a wrapper on the iterator rather than on
by default, though it is now on in every one of our integration test cases.
Test plan:
- Added to the integration tests for full coverage.
Remove the CodeGen v1 sections of the CI config because both OID and OIL use
CodeGen v2.
We were missing running any test that wasn't `Oi{d,l}Integration.*` before.
This now runs the unit tests again and requires a minor fix to one unit test.
Test plan:
- CI
We previously moved container identification later in CodeGen in order
to preserve information for AlignmentCalc.
However, Flattener needs to know if a class is a container in order to
apply its special handling for this case.
This new approach moves container identification in front of Flattener,
but has Container own a type node, representing its layout. This
underlying type node can be used for calculating a container's
alignment in a later pass.
They must not appear in the final generated code as we'd end up with
invalid types with void members, e.g.:
struct Foo {
int a;
void myIncompleteMember;
int c;
};
Removing them from the type graph early also ensures that padding is
calculated correctly.
Summary:
Remove the now useless `handler` and adds the `traversal_func` and `processor`
entries for `std::list`. This type is a bit weird as most of our sequential
containers don't have any overhead on storing the element. I went for the same
approach we take for maps where we have a shared `[]` element covering the map
overhead and below that a `key` & `value`. As we only have a single element
under it which doesn't have a logical name I went for `*`.
Closes#315.
Test Plan:
- CI
- Copied the relevant `std::vector` tests and updated the existing one.
Summary:
Currently there are two features between CodeGen v2 (TypeGraph) and TreeBuilder
v2. These are TypedDataSegment and TreeBuilderTypeChecking. Each of these
features currently has a full set of tests run in the CI and each have specific
exclusions.
Collapse these features into TreeBuilder v2. This allows for significantly
simplified testing as any OIL tests run under TreeBuilder v2 and any OID tests
run under TreeBuilder v1.
The reasoning behind this is I no longer intend to partially roll out this
feature. Full TreeBuilder v2 applies different conditions to containers than
the intermediate states, and writing these only to have them never deployed is
a waste of time.
Test Plan:
- it builds
- CI
Not all containers have 8-byte alignment, so if we want to avoid lots of
manual logic for calculating container alignment on a case-by-case
basis, we must calculate alignment from the member variables before the
Class nodes have been replaced by Container nodes.
Leave it to the new mutator pass IdentifyContainers to replace Class
nodes with Container nodes where appropriate.
This will allow us to run passes over the type graph before identifying
containers, and therefore before we have lost information about the
internal details of the container (e.g. alignment of member variables).
For the containers which are allowed to be declared with incomplete
types, it is only the contained types which are allowed to be
incomplete. Other template parameters (e.g. allocators) must always be
defined before use.
Summary:
Adds the option for required features to container definitions. These cause the container not to be passed to `DrgnParser` if that feature is not enabled during code generation. The thrift isset type does not currently work with `tree-builder-v2` and only provides benefit with `capture-thrift-isset`. This change makes sure the container is ignored if it won't be useful, allowing code generation under `tree-builder-v2`.
Test Plan: - CI
Differential Revision: D49960512
Pulled By: JakeHillion
