Conversation
* init alias analysis for alloc Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * add MemFree events to the test Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * add first test for alias analysis Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * add second test for alias analysis Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * improve field insensitive for alloc Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * add third test for alias analysis Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * add alias analysis for free events Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * use alias analysis for alloc and free in relation analysis Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * change native relation analysis Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * change logic of mustAlias Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * Using stack pointer in alloc tests * change exception type Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * Add full alias analysis Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * Add object alias for alloc -> memory event Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> * Add alloc and free to alias graph Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> --------- Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com> Co-authored-by: Tianrui Zheng <tianrui.zheng@huawei.com> Co-authored-by: Natalia Gavrilenko <natgavrilenko@gmail.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
| .withConfig(configuration) | ||
| .withTarget(target); | ||
| Program program = new ProgramParser().parse(new File(getTestResourcePath("alloc/" + name + ".ll"))); | ||
| Wmm mcm = new ParserCat().parse(new File(getRootPath("cat/vmm.cat"))); |
There was a problem hiding this comment.
Let's add tests for all the three models: vmm, rc11, and c11. I think expected results should be different for all of them, so, instead of listing all variations explicitly, we can loop through the models or create a separate unit test for each model.
There was a problem hiding this comment.
All the tests we have so far have same results for the three models. Do you mean we will have some tests that pass for some models but fail for others?
There was a problem hiding this comment.
I mean that it is very unlikely that we will have tests that should pass for one model and fail for the other, so we can use the same expected results for all models.
There was a problem hiding this comment.
I see.
I think expected results should be different for all of them
It confused me a bit :-)
| @@ -0,0 +1,117 @@ | |||
| package com.dat3m.dartagnan.c; | |||
There was a problem hiding this comment.
The package is "com.dat3m.dartagnan.llvm"
cat/c11.cat
Outdated
| flag ~empty (allocmem \ hb) as use-before-alloc | ||
| flag ~empty ((allocmem^-1 ; (allocptr & (ALLOC * FREE))) \ hb) as use-after-free | ||
|
|
||
| // TODO: Non-monotonic! (see ThreadCreation class) |
There was a problem hiding this comment.
Thread join has been fixed, so this TODO can be removed for all three models
| import java.util.HashSet; | ||
| import java.util.Set; | ||
|
|
||
| // TODO: Consistent naming. Should we rename relation 'Free' into FreeRel? |
There was a problem hiding this comment.
Let's align the naming and remove the TODO. Any reasonable naming of your choice should be fine.
| - Side-effectful non-termination requires at least one infix iteration | ||
| - Side-effect-free non-termination causes exactly one suffix iteration. | ||
| */ | ||
| private BooleanFormula encodeInfixSuffixDecomposition() { |
There was a problem hiding this comment.
All these methods can be changed back to private
| BooleanFormula encoding = (specType == Property.Type.SAFETY) ? | ||
| encodePropertyViolations(properties) : encodePropertyWitnesses(properties); | ||
| if (!program.getFormat().equals(LLVM) || properties.contains(TERMINATION)) { | ||
| // if (!program.getFormat().equals(LLVM) || properties.contains(TERMINATION)) { |
| import com.dat3m.dartagnan.program.Thread; | ||
| import com.dat3m.dartagnan.program.*; | ||
| import com.dat3m.dartagnan.program.event.*; | ||
| import com.dat3m.dartagnan.program.event.core.Assume; |
| // getOrCreatePredefinedRelation(IDDTRANS), | ||
| // addDefinition(product(newRelation(), Tag.MEMORY, Tag.MEMORY)) | ||
| // ); | ||
| case DATA -> { |
There was a problem hiding this comment.
This works correctly but doesn't look very clean. I think at this point we should consider if:
- Alloc and Free events should have Tag.MEMORY
- Alloc and Free events should inherit MemoryEvent or MemoryCoreEvent.
I didn't look into this myself yet, but we should definitely check what would be the implications of this. It might be that it will also make alias analysis a bit cleaner and give us ordering via the co relation (if Alloc and Free are a part of the co chain). It might be also that it is not going to work for some reason.
There was a problem hiding this comment.
We can also change Tag.MEMORY to Tag.VISIBLE, and it should include all the necessary events
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
|
Could you change this merge request to use development branch as a base? Otherwise, there are many changes that are not related to alloc-free |
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
| @@ -1,51 +1,49 @@ | |||
| ; ModuleID = 'dglm.c' | |||
| source_filename = "dglm.c" | |||
| ; ModuleID = 'benchmarks/lfds/dglm.c' | |||
There was a problem hiding this comment.
We also need to update .ll files for dglm-CAS-relaxed, ms-CAS-relaxed, and trebier-CAS-relaxed. It should probably be possibly to figure out what exactly was relaxed if you compare the relaxed .ll file with a the original non-relaxed .ll file.
| @@ -1,376 +1,307 @@ | |||
| ; ModuleID = '/home/ponce/git/Dat3M/output/clh_mutex.ll' | |||
| source_filename = "/home/ponce/git/Dat3M/benchmarks/locks/clh_mutex.c" | |||
| ; ModuleID = 'benchmarks/locks/clh_mutex.c' | |||
There was a problem hiding this comment.
Same here, we should also update the alternative acq2rx files
benchmarks/alloc/test1_err_address.c
Outdated
| arr[0] = 0; | ||
| arr[1] = 1; | ||
|
|
||
| return NULL; |
There was a problem hiding this comment.
Minor thing: it looks like the indentation is too large here and in several other places
| @@ -0,0 +1,26 @@ | |||
| #include <stdlib.h> | |||
There was a problem hiding this comment.
Since all tests here start with 'test1_', we can rename them to just 'test_'
benchmarks/lfds/dglm.h
Outdated
| _Atomic(Node *) Tail; | ||
| _Atomic(Node *) Head; | ||
|
|
||
| #define RETIRE_THRESHOLD 10 |
There was a problem hiding this comment.
I think it is better not to modify the original algorithm with our custom retirement method but do the following:
- Keep a 'normal' free and add a comment that it should retire instead
- For the VMMLFDSTest which fails, splits it into three tests: one for PROGRAM_SPEC, one for TERMINATION, and one for CAT_SPEC.
- For the CAT_SPEC version of the tests, change expected result to 'FAIL' and add a comment that it fails use-after-free because it requires safe memory reclamation.
There was a problem hiding this comment.
Now we need AllocMemGraph and AllocPtrGraph because lack of these two makes testing with only one property PROGRAM_SPEC or TERMINATION impossible.
| encoding = bmgr.and(encoding, encodeProgramTermination()); | ||
| Event termination = program.getThreadEvents(Termination.class).stream().findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException("Malformed program: missing the termination event")); | ||
| encoding = bmgr.and(encoding, context.execution(termination)); |
| ImmutableSet.Builder<Location> addresses = ImmutableSet.builder(); | ||
| Collector collector = new Collector(e.getAddress()); | ||
| addresses.addAll(collector.address()); | ||
| protected ImmutableSet<Location> getAddressSpace(Event e) { |
| } | ||
|
|
||
| @Override | ||
| // TODO: May and must sets of AllocMem can become very large for some programs. |
There was a problem hiding this comment.
Maybe we can also try to fix this. As far as I see, in the alias analysis, the only not thread-safe part is the EqualityAliasAnalysis (assuming that it can be called from different threads only after the initialization stage).
The problem is that EqualityAliasAnalysis computes trueSet and falseSet on demand, and that's why it can have a concurrent modification. If we precompute that sets in the constructor (and actually we will only need the trueSet), then it should be safe to use the lazy relation analysis here. Looks like it is a relatively small change that can solve a potentially huge performance issue.
| // ); | ||
| case DATA -> { | ||
| Relation mm = addDefinition(product(newRelation(), Tag.MEMORY, Tag.MEMORY)); | ||
| Relation am = addDefinition(product(newRelation(), Tag.ALLOC, Tag.MEMORY)); |
There was a problem hiding this comment.
We can combine tags in a filter instead of creating a separate relation for each tag, something like this
case DATA -> intersection(r, getOrCreatePredefinedRelation(IDDTRANS),
addDefinition(new CartesianProduct(newRelation(),
Filter.union(Filter.byTag(Tag.MEMORY), Filter.byTag(Tag.ALLOC)),
Filter.byTag(Tag.MEMORY))));
Please also delete the commented out code
| .forEach(this::instrumentLoop); | ||
| } | ||
|
|
||
| program.addTerminationThread(); |
There was a problem hiding this comment.
Ideally, we should optimize this and create a a termination thread only when really needed:
- the original condition when encodeProgramTermination() was called in PropertyEncoder, or
- if property is CAT_SPEC and there is at least one Alloc event in the program
But I don't immediately see what would be an elegant way to get verification property in a ProgramProcessor. I will try to find a clean solution for this.
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
Signed-off-by: Tianrui Zheng <tianrui.zheng@huawei.com>
No description provided.