Add LLVM-16 to the Nix builds and CI matrix. Also add the option to explicitly
select an LLVM version so the CircleCI build can still work as before.
We should support at least LLVM-17 currently, but there appears to be a
significant performance regression in compiling the generated code for large
variants that I'm going to debug separately. The long term goal is to support
and test all versions 15 and up.
Test plan:
- CI
OI's build is challenging and has often been a problem for the Open Source
community. It requires an extremely specific set of dependencies that are very
hard to achieve on most systems. There are frequent breakages, like when
updating to CentOS Stream 9, or when trying to update the CI's clang from
clang-12 to clang-15 - OI requires the clang libraries to be version 15 but
can't be compiled with it on the CI!
This changes provides a mostly working build environment with `nix`. This
environment is pinned to a specific nixpkgs revision using `flake.lock`, and
only updates when we explicitly tell it to.
Summary of changes:
- Update CMakeLists.txt required version to 3.24. This allows specifying
`FIND_PACKAGE_ARGS` in `FetchContent`, meaning we can use system packages.
This is available on most up to date distros (3.30.2 is current).
- Extends `flake.nix` to be able to build OI. Adds instructions for building
and developing OI using `nix`.
- Partially runs the tests in GitHub Actions. A huge amount must be excluded
because of incompatibilites between the clangStdenv compiler and drgn. We
have similar, though fewer, issues when building with the clang-12/libstdcxx
mix on the Ubuntu 22.04 CircleCI, though this is at least reproducible.
- Updates CircleCI to build CMake from source as we don't have a newer image
available. Also add some newly found dependencies (not sure how it was
working without them before).
Test plan:
This change requires less testing than previous build related changes because
it deprecates most of the build types.
- The internal BUCK build is unaffected. No special testing.
- The semi-internal CMake build is gone. Use Nix.
- The Nix build for clang-15 and some tests are continuously tested in GitHub
actions.
- Tested the set of Nix commands in the README. All work except the one that
points to GitHub as this must be merged first.
- The existing CircleCI runs on Ubuntu 20.04 are maintained.
- Unable to test the new `test-report.yml` as it must be merged due to the
permissions it needs. Will follow up with testing after this is merged. See:
https://github.com/dorny/test-reporter?tab=readme-ov-file#recommended-setup-for-public-repositories
The list of exclusions for GitHub Actions/nix testing is currently very long, I
think 29% of the tests. This should be stable and reproducible though, and
likely needs deep changes to OI to fix. That's why fixes are excluded from this
PR. It's all to do with the forked drgn not being able to parse clang's newer
DWARF output, and can't be fixed by rolling back as we required a relatively
new libcxx.
Summary:
Pull Request resolved: https://github.com/facebookexperimental/object-introspection/pull/503
The internal build is moving to LLVM-17 from LLVM-15. For our code there are two required changes to get it building: a header has been renamed and the signature of a function has changed.
Checked which LLVM major version these changes apply in and both are 16, macro gated accordingly so we maintain compatibility with both LLVM-15 and LLVM-17. Both versions build internally.
Differential Revision: D61268962
Currently we pull a modified folly repository to be able to use it header only.
The only difference in this repository is adding the `folly-config.h` file.
Pull this patch into this repo instead and use upstream folly.
Bump folly to latest main as well.
Test plan:
- CI build. I have local build issues at the moment.
Currently in TypeGraph when generating inst::Field objects in the
generated source we use the `sizeof` operator to construct the static
and exclusive size. As you can't use sizeof() on a bitfield this
generates invalid code. This fix special cases bit fields and sets the
static and exclusive size to 0 as there size will be rolled up in the
parent object.
If a class inherits from more than one base class Clang Parser currently
calculates incorrect offsets for everything but the first base class.
This is owing to the fact that TypeGraph needs offsets in bits but
ClangParser is providing them in bytes.
Summary:
The container implementation for `folly::Optional` currently lacks TBv2 support. This change is a skilfully crafted cut-n-paste of the `std::optional` TBv2 implementation.
Reviewed By: ajor
Differential Revision: D56139280
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
Start the migration from CircleCI to GitHub Actions with migrating the lint
job. Used the structure from @robandpdx to setup Nix and use a GitHub key.
Restructured the check from `nix flake check` to
`nix fmt; git diff --exit-code` so we get a full patch again.
Test plan:
- Submitted this PR with a formatting error. CI failed. Submitted without and
it passed.
Co-authored-by: Rob Anderson <robandpdx@github.com>
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
Summary:
tbv2: remove unnecessary copy in Element
`IntrospectionResult::const_iterator` iterates through the `Element`s in an
`IntrospectionResult`. `Element` currently copies the `type_path` which is a
`std::vector<string_view>` every time the iterator is incremented. This is
unnecessary as the data in the vector only changes slightly between iterations.
This change changes the `type_path` field in `Element` to a
`std::span<const std::string_view>`. Doing this previously caused SEGVs because
of the iterator's potential to be copied. To make it possible we do two things:
1. Make all copies explicit using a clone interface as in `ContainerInfo`. This
prevents accidental copies of an expensive structure.
2. After calling the copy constructor in `clone()` update the `span` in `next_`
to point at the newly copied structure.
Moves are fine because the `span` points at the allocation of the `vector`, not
the vector itself.
Test Plan:
- CI
- `FILTER='OilIntegration.*' make test`
- Ran `OilgenIntegration.std_vector_vector_int_some` which SEGVd with the
`span` change before and now doesn't. This now passes cleanly with ASAN
enabled on the target, though isn't available in `main` (only works on my
machine).
Differential Revision: D53472595
Pulled By: JakeHillion
Summary: Adding support for the nullptr clang type (clang::BuiltinType::NullPtr). While there I augmented the exception message to include the type name that is missing.
Differential Revision: D53272742
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.
* fix string type sso computation
* fix rest of sbo/sso calculation
* make placement of uintptr_t cast consistent
* separate check-inline into function
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
Previously AlignmentCalc calculates the alignment and sets packing for every
type except a member with explicit alignment. Change this to check whether an
alignment has been previously set for a type before calculating it. Use this in
ClangTypeParser where the full alignment of the type is available.
Remove explicitly aligning members by the type because that was previously
reserved for members with explicit alignment. AlignmentCalc will correctly
align a member to the underlying type without this. Explicit member alignment
is still missing, as before this change.
Test plan:
- CI
- Too little. Gets further into a production type than without this change.
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.
Some of the logic that makes Thrift isset work for TreeBuilder-v2 in DrgnParser
(JIT OIL) wasn't ported to ClangTypeParser meaning it doesn't work in
Ahead-of-Time (AoT) OIL.
Add the template parameter name reconstruction for enum values to
ClangTypeParser.
Test plan:
- Tested with Thrift isset enabled on an internal type. Doesn't build before,
does build after.
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
TreeBuilder did not consider a zero-length array like a container and
never read the array's sizeof stored in the data buffer, leading to a
mismatch between bytes written vs read out of the buffer.
Now, `TreeBuilder::isContainer` does consider zero-length array like
a container and properly consume all the object sizes in the buffer.
