The CloneContext will currently de-duplicate any pointers that are cloned multiple times, however this isn't ideal behavior for AST nodes.
AST nodes must be unique in the tree, so a non-transform clone of the AST from one program to another should only ever Clone() called once per node. Hitting the map for these is inefficent, worse still, there are cases in the transforms where we actually *want* to create N copies of the same source node. The current implementation makes this impossible.
This change introduces ShareableCloneable, a new base class that derives from Cloneable.
Repeated calls to CloneContext::Clone() for Cloneable pointer types will now produce a new, unique instance per call.
Repeated calls to CloneContext::Clone() for ShareableCloneable pointer types will de-duplicate as before.
type::Type objects are shared, want deduplication while cloning, so now derive from ShareableCloneable.
ast::Node continues to derive from Cloneable.
Bug tint:469
Change-Id: I5ca906d507de6271d5d715cfdc962a55b721e821
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47776
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
Almost all transforms should clone all symbols before doing any work,
to avoid any newly created symbols clashing with existing symbols the
source program and causing them to be renamed.
The Renamer is the exception to this, and so an optional flag is used
to prevent automatic cloning of symbols for this transform.
Bug: dawn:758
Change-Id: I84527a352825b2eaa43eabe225beb9e0999bf048
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48000
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Ben Clayton <bclayton@chromium.org>
This will be used to detect accidental leaks of program objects between programs.
Bug: tint:709
Change-Id: I20f784a2c673d19a04a880b3ec91dfe2eb743bdb
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47622
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
SymbolTable::New() used to build and return a symbol without a registered name. When you asked for the name of the symbol it would return tint_symbol_N, where N is the numerical identifier for the symbol.
This approach was a major tripping hazzard for transforms that liked to fetch the source program name, and register it in the new program (in this situation, you should always use `CloneContext::Clone(Symbol)`).
Without special casing for unnamed symbols, you could end up promoting the unnamed symbol to a named symbol, and then colliding against a new unnamed symbol.
This is exactly what happened in tint:711.
Instead, with this change:
* The concept of unnamed symbols has been removed. All symbols now have a name.
* The signature of `SymbolTable::New()` has been changed to take a name parameter (which defaults to 'tint_symbol'). This can be used to create a new, unique named symbol (possibly with a suffix), which will not collide with any existing symbols. Note these symbols may still collide if `SymbolTable::Register()` is called with the same name. All Transforms that currently use `SymbolTable::Register()` will be fixed in another change.
* The CloneContext has been updated to use `SymbolTable::New()` instead of `Register()`. This means that any symbols defined before a clone will not collide.
* `CloneContext::CloneSymbols()` has been added which allows a transform to pre-clone all the symbols from the source program. This can be used to avoid the authored identifiers being suffixed with a number, in the case a transform calls New() before the symbol is cloned.
* `Symbol::to_str()` has been changed to return `$<id>` instead of `tint_symbol_N`. This is to avoid any confusion between the actual name and the symbol ID.
Bug: tint:711
Bug: tint:712
Change-Id: I526e4b49b7027545613859de487e6a275686107a
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47631
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Registering a new Symbol with the NameFor() of the source symbol creates
a new *named* symbol. When mixing these with unnamed symbols we can have
collisions.
Update CloneContext::Clone(Symbol) to properly clone unnamed symbols.
Update (most) the transforms to ctx.Clone() the symbols instead of
registering the names directly.
Fix up the tests where the symbol IDs have changed.
Note: We can still have symbol collisions if a program is authored with
identifiers like 'tint_symbol_3'. This will be fixed up in a later
change.
Change-Id: I0ce559644da3d60e1060f2eef185fa55ae284521
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46866
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
There's usually only ever one vector we want to insert into.
Inserting into *all* vectors that happen to contain the reference object is likely unintended, and is a foot-gun waiting to go off.
Change-Id: I533084ccad102fc998b851fd238fd6bea9299450
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46445
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
The readers must not produce invalid ASTs.
If readers cannot produce a valid AST, then they should error instead.
If a reader does produce an invalid AST, this change catches this bad behavior early, significantly helping identify the root of the broken logic.
IsValid() made a bit more sense in the days where the AST was mutable, and was constructed by calling setters on the nodes to build up the tree.
In order to detect bad ASTs, IsValid() would have to perform an entire AST traversal and give a yes / no answer for the entire tree. Not only was this slow, an answer of 'no' didn't tell you *where* the AST was invalid, resulting in a lot of manual debugging.
Now that the AST is fully immutable, all child nodes need to be built before their parents. The AST node constructors now become a perfect place to perform pointer sanity checking.
The argument for attempting to catch and handle invalid ASTs is not a compelling one.
Invalid ASTs are invalid compiler behavior, not something that should ever happen with a correctly functioning compiler.
If this were to happen in production, the user would be utterly clueless to _why_ the program is invalid, or _how_ to fix it.
Attempting to handle invalid ASTs is just masking a much larger problem.
Let's just let the fuzzers do their job to catch any of these cases early.
Fixed: chromium:1185569
Change-Id: I6496426a3a9da9d42627d2c1ca23917bfd04cc5c
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/44048
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Reviewed-by: David Neto <dneto@google.com>
All includes from .cc to .h are preserved, even when transitively included.
It's clear that there are far too many includes in header files, and we should be more aggressive with forward declarations. tint:532 will continue to track this work.
There are, however, plenty of includes that have accumulated over time which are no longer required directly or transitively, so this change starts with a clean slate of *required* includes.
Bug: tint:532
Change-Id: Ie1718dad565f8309fa180ef91bcf3920e76dba18
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/44042
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Will be used by a Renamer transform
Change-Id: Ic0e9b69874f51103f0beec7745d32a9f8419e93a
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/42841
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
to TINT_INSTANTIATE_TYPEINFO()
ClassID isn't a thing any more.
Change-Id: Ie1c0d4a95e58ef7166d3cab5ef733a2dfc702345
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/42921
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
You have to have the CloneContext in order to call ReplaceAll() in the first place. The overhead of capturing the pointer in the closure is negligible.
Cleans up the callsites.
Change-Id: I3a0fd808517d69f19756f590f3426e5ba226c57e
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/42840
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Add Source::FileContent to hold the file source content and per-line data.
Have Source hold an optional pointer to a FileContent, and add a file_path field.
This allows us to kill the `FreeInternalCompilerErrors()` filth as we're now able to construct Sources that hold a file path without file content.
Change-Id: I03556795d7d4161c3d34cef32cb685c45ad04a3d
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/42026
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Commit-Queue: Ben Clayton <bclayton@google.com>
Chromium has its own test main() entrypoint.
To ensure that Chromium doesn't panic about memory leaks with the tests that exercise the ICE cases, we have to explicitly call the FreeInternalCompilerErrors() functions in these tests (at least until I can add this to end of Chromium's test main() function)
Change-Id: I2ea5109fcdb5f68f56a19709a1ec35ed72c0f760
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/42025
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Commit-Queue: Ben Clayton <bclayton@google.com>
Appends an error message with the tint compiler source location to the
provided diagnositic list, and then calls the global error handler if
one is set.
Tests and the sample app now register an error handler to print the
diagnostic list to stderr and abort when NDEBUG is not defined.
All uses of assert(false) have been fixed up to use these macros.
Change-Id: I2f63e51ed86ac23883301d280070bd1a357c6cb2
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/41620
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
The CloneContext was previously dealing with pointers to CastableBase, which has no guarantees that the object was actually cloneable.
Add a Cloneable base class that CloneContext can use instead.
Improves readability and produces cleaner compiler errors if you try to clone a non-cloneable object.
Change-Id: I4352fc5dab3da434e4ab160a54c4c82d50e427b4
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/41722
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Inserts objects before others when cloning
Change-Id: Ibf247abae3aeb3d351048f1182db2a2b42b2c677
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/41547
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: David Neto <dneto@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
And assert that the cast succeeded.
There is a danger with Replace() or ReplaceAll(), where you can end up replacing a node with another node of an incompatible type for some reference of that object. Previously this would silently cast to the incorrect type, and Bad Things would happen. Now we will assert in this situation.
I have not observed this issue happening (all current uses of Replace() and ReplaceAll() are believed to be safe). This is just an edge case I've spotted and wanted to add some safety belts for.
Change-Id: Icf4a4fe76f7bc14bcc6b274de68f7d0b3d85d71f
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/41546
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Reviewed-by: David Neto <dneto@google.com>
Will hold the mutable fields that currently reside in the otherwise immutable-AST.
Change the AST string methods to accept a `const semantic::Info&`. This is required as some nodes include type-resolved information in their output strings.
Bug: tint:390
Change-Id: Iba494a9c5645ce2096da0a8cfe63a4309a9d9c3c
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/39003
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Program is now immutable*, and remains part of the public Tint
interface.
ProgramBuilder is the mutable builder for Programs, and is not part of
the public Tint interface. ast::Builder has been folded into
ProgramBuilder.
Immutable Programs can be cloned into a mutable ProgramBuilder with
Program::CloneAsBuilder().
Mutable ProgramBuilders can be moved into immutable Programs.
* - mostly immutable. It still has a move constructor and move
assignment operator - required for practical usage - and the
semantic information on AST nodes is still mutable.
Bug: tint:390
Change-Id: Ia856c50b1880c2f95c91467a9eef5024cbc380c6
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/38240
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Instructs the clone context to replace a single object instance with a given replacement.
Will be used to fix brokenness in transforms.
Bug: tint:390
Change-Id: I17bf1cdf7549f697281ca7c286bdb5771e5a6f1a
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/38553
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Enforce all places where Dawn passes in or returns a ast::Module, now takes a `const Program* ` or returns a `Program`.
As the end goal of all this is to have immutable Programs, all Program inputs take a pointer instead of moving the actual object.
As consumers of a Program are now all const, we have to const_cast to work around all the places we've been incorrectly mutating a ast::Module.
These const_casts are temporary, and will be fixed in the next set of changes.
Depends on https://dawn-review.googlesource.com/c/dawn/+/38522
Bug: tint:390
Change-Id: Ie05b112b16134937d1b601e9b713ea4ec4e1c677
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/38541
Reviewed-by: dan sinclair <dsinclair@chromium.org>
In the future, CloneContext will be operating on `Program`s so a field called `mod` is poorly named.
CloneContext has a `src` member, so rename to `dst` to keep symmetry.
Bug: tint:390
Change-Id: Ic724f8a18b46ef719790394cdc810f7eb3681234
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/38364
Commit-Queue: David Neto <dneto@google.com>
Auto-Submit: Ben Clayton <bclayton@google.com>
Reviewed-by: David Neto <dneto@google.com>
CloneContext clones the AST, types, symbols and in the future semantic info.
3/4 of these are non-ast, so promote these up to the root.
Bug: tint:390
Change-Id: I49619796e6f81f9ab64f79413a12c87312cb1901
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/38361
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Ben Clayton
Antonio Maiorano
James Price