Closed typesanitizer closed 4 years ago
This should be a good starter task, as the behavior after the refactoring should not change. There is no new functionality to be added. The main challenge would be to get the compiler building properly and being careful while making individual changes.
Please feel free to ask for help or code review (I'm varungandhi-apple on GitHub). 🙂
@swift-ci create
Comment by Matei Oprea (JIRA)
Let's try to fix this. Any hints? I know that it might be difficult jumping from SR-13237 to this, but I'll try.
I'm not sure what other hints I can provide, I wrote down what I thought might help in the description. � Is something in the description unclear? Happy to answer more specific questions.
Comment by Matei Oprea (JIRA)
Does it makes sense to use totalSizeToAlloc when context.Allocate only allocs the size of type?
E.g.:
void *mem = ctx.Allocate(sizeof(DifferentiableAttr), alignof(DifferentiableAttr));
@dan-zheng, Matei ran into the question above ^. I would prefer that we consistently use one pattern to allocate throughout the codebase, where we specify all the trailing objects with their counts, even if the counts are zero. So these two pieces of code would change slightly:
// Before
DifferentiableAttr::create(ASTContext &context, ...)
size_t size = totalSizeToAlloc<ParsedAutoDiffParameter>(parameters.size());
void *mem = context.Allocate(size, alignof(DifferentiableAttr));
DifferentiableAttr::create(AbstractFunctionDecl *original, ...)
void *mem = ctx.Allocate(sizeof(DifferentiableAttr),
alignof(DifferentiableAttr));
// After (ignore formatting/line-width)
DifferentiableAttr::create(ASTContext &context, ...)
unsigned size = totalSizeToAlloc<DifferentiableAttr, ParsedAutoDiffParameter>(0, parameters.size());
void *mem = context.Allocate(size, alignof(DifferentiableAttr));
DifferentiableAttr::create(AbstractFunctionDecl *original, ...)
size_t size = totalSizeToAlloc<DifferentiableAttr, ParsedAutoDiffParameter>(1, 0);
void *mem = ctx.Allocate(size, alignof(DifferentiableAttr));
Arguably, this also makes it easier to compare the two calls at a glance.
Does this sound like a reasonable change?
Yes, that sounds reasonable!
Comment by Matei Oprea (JIRA)
Just curious, shouldn't be like:
DifferentiableAttr *
DifferentiableAttr::create(AbstractFunctionDecl *original, bool implicit,
SourceLoc atLoc, SourceRange baseRange, bool linear,
IndexSubset *parameterIndices,
GenericSignature derivativeGenSig) {
auto &ctx = original->getASTContext();
size_t size = totalSizeToAlloc<ParsedAutoDiffParameter>(0);
void *mem = ctx.Allocate(size, alignof(DifferentiableAttr));
Since the definition says that it includes the size of the base object.
Also, what I'm trying to understand is: why doesn't the code compile when I'm passing it totalSizeToAlloc\<DifferentiableAttr>(0); since I'm trying to alloc sizeOf(DifferentiableAttr), not sizeOf(ParsedAutoDiffParameter), right?
You're right mateioprea (JIRA User) , good catch! It should be totalSizeToAlloc<ParsedAutoDiffParameter>(arg);
. where arg = 0
or arg = parameters.size()
.
> Also, what I'm trying to understand is: why doesn't the code compile when I'm passing it totalSizeToAlloc\<DifferentiableAttr>(0);
There is an explanation in the comment above additionalSizeToAlloc
which shares a similar type signature as totalSizeToAlloc
/// Returns the size of the trailing data, if an object were
/// allocated with the given counts (The counts are in the same order
/// as the template arguments). This does not include the size of the
/// base object. The template arguments must be the same as those
/// used in the class; they are supplied here redundantly only so
/// that it's clear what the counts are counting in callers.
template <typename... Tys>
static constexpr typename std::enable_if<
std::is_same<Foo<TrailingTys...>, Foo<Tys...>>::value, size_t>::type
additionalSizeToAlloc(typename trailing_objects_internal::ExtractSecondType<
TrailingTys, size_t>::type... Counts) {
template <typename... Tys>
static constexpr typename std::enable_if<
std::is_same<Foo<TrailingTys...>, Foo<Tys...>>::value, size_t>::type
totalSizeToAlloc(typename trailing_objects_internal::ExtractSecondType<
TrailingTys, size_t>::type... Counts) {
Comment by Matei Oprea (JIRA)
Perfect, thank you. I already have some of the changes done (with tests passing). I'll get back to you when everything is done.
Meanwhile, maybe this is not on your side, but I've tried to also fix SR-11900 and ping Brent on twitter too, but no answer. Can you help with this one too? Or it's not on your side?
> Meanwhile, maybe this is not on your side, but I've tried to also fix SR-11900 and ping Brent on twitter too, but no answer
I'm not familiar with the details of that bug report.
I don't think we have a policy on this, but speaking for myself, I would prefer that someone contact me using the bug reporting channel and check back in a few days if they don't get a response (sometimes I might be busy with other work and miss something), or ask for help from other contributors in the Compiler Development category of the Swift forums, instead of pinging me on Twitter.
Comment by Matei Oprea (JIRA)
Understood. Thanks!
Comment by Matei Oprea (JIRA)
Hi Varun,
Since some of the compiler classes do not extend the TrailingObjects class as a friend class, I assumed that this requires more than a "StarterBug" experience.
If this fix should cover all manual allocations, even the classes that do not extend TrailingObjects, I'll try to do it.
I've also created a PR and mentioned you.
Also, ran the tests on my local env and all passed.
Thanks.
PR link: https://github.com/apple/swift/pull/33256
(I've answered the questions in the PR).
Additional Detail from JIRA
| | | |------------------|-----------------| |Votes | 0 | |Component/s | Compiler | |Labels | Task, StarterBug | |Assignee | mateioprea (JIRA) | |Priority | Medium | md5: 7858934d4383a96215477b451bd3e615Issue Description:
There are a bunch of places in the compiler which use manual size calculations to allocate memory. Here's an example from
ImportCache::getImportSet
The manual calculations are not necessary. There is a helper method
totalSizeToAlloc
onTrailingObjects
for precisely this use-case. Here's a (somewhat complex) example fromSILFunctionType::get
Following this, the earlier calculation could be replaced with (ignore the slightly odd formatting)
We should replace this calculation and other similar manual calculations (you'll find them in the vicinity of calls to
.Allocate
methods) with appropriate calls tototalSizeToAlloc
.Note that in case you make a mistake here, you will probably get some memory corruption issue which will not be fun to debug. I have a couple of suggestions here:
1. Make changes to one place in one commit. This will make bisecting easier in case you hit an issue later. (The commits can be squashed later on, either before submitting the PR or after the review is done and before merging.)
enable-asan flag), and maybe even UBSan (-enable-ubsan). The way to make changes would be first test the baseline (master). Then after every commit, make sure that all the tests pass with the ASan-ified compiler.So long as you are systematic and don't make a typo or something, hopefully you won't hit any issues.
The PR should probably run the source compat suite as well, in case something breaks.