Fix ASAN/UBSAN issues in DimAnalysis #3052
Conversation
jorickert
commented
Jan 28, 2025
jorickert
commented
- Fixes a memory leak
- Fixes an integer overflow caused by a dynamic shape
- Fixes reshape to wrong type in LIT tests
- Fixes a memory leak - Fixes an integer overflow caused by a dynamic shape - Fixes reshape to wrong type in LIT tests Signed-off-by: Rickert, Jonas <[email protected]>
jorickert
force-pushed
the
jrickert.upstream.asan_dim_analysis
branch
from
8f49028 to
7ad9f4f
Compare
|
@tungld do you mind reviewing this PR, its in dim analysis. Thanks |
| @@ -361,11 +361,11 @@ static bool exploreSameDimsUsingShapeHelper(const DimAnalysis::DimT &dim, | |||
| ONNXOpShapeHelper *shapeHelper = | |||
| shape_op.getShapeHelper(op, {}, nullptr, nullptr); | |||
| // If no shape helper, or unimplemented, just abort. | |||
| if (!shapeHelper || !shapeHelper->isImplemented()) | |||
| if (!shapeHelper) | |||
There was a problem hiding this comment.
Could you explain what's wrong with calling shapeHelper->isImplemented()?
There was a problem hiding this comment.
There is nothing wrong with it, but if the shape helper is not implemented we still need to delete it or we will leak it's memory. This happens a few lines down
There was a problem hiding this comment.
I got it! That's why we have to distinguish the two conditions. Thanks for the explanation!
| } | ||
| for (int64_t i = 0; i < outputType.getRank(); ++i) { | ||
| outputStaticSize *= | ||
| outputType.isDynamicDim(i) ? -1 : outputType.getShape()[i]; |
| %0 = "onnx.Reshape"(%arg0, %shape) {allowzero = 0 : si64} : (tensor<8x?x16x4xf32>, tensor<3xi64>) -> tensor<?x4x32xf32> | ||
| %1 = "onnx.MatMul"(%arg0, %0) : (tensor<8x?x16x4xf32>, tensor<?x4x32xf32>) -> tensor<8x?x16x32xf32> | ||
| "onnx.Return"(%1) : (tensor<8x?x16x32xf32>) -> () | ||
| %0 = "onnx.Reshape"(%arg0, %shape) {allowzero = 0 : si64} : (tensor<8x?x16x4xf32>, tensor<3xi64>) -> tensor<?x4x128xf32> |
| @@ -361,11 +361,11 @@ static bool exploreSameDimsUsingShapeHelper(const DimAnalysis::DimT &dim, | |||
| ONNXOpShapeHelper *shapeHelper = | |||
| shape_op.getShapeHelper(op, {}, nullptr, nullptr); | |||
| // If no shape helper, or unimplemented, just abort. | |||
| if (!shapeHelper || !shapeHelper->isImplemented()) | |||
| if (!shapeHelper) | |||
There was a problem hiding this comment.
I got it! That's why we have to distinguish the two conditions. Thanks for the explanation!
|
Jenkins Linux amd64 Build #16169 [push] Fix ASAN/UBSAN issues in... started at 01:59 |
|
Jenkins Linux ppc64le Build #15199 [push] Fix ASAN/UBSAN issues in... started at 03:19 |
|
Jenkins Linux s390x Build #16172 [push] Fix ASAN/UBSAN issues in... started at 03:00 |
|
Jenkins Linux amd64 Build #16169 [push] Fix ASAN/UBSAN issues in... passed after 1 hr 23 min |
|
Jenkins Linux s390x Build #16172 [push] Fix ASAN/UBSAN issues in... passed after 1 hr 40 min |
|
Jenkins Linux ppc64le Build #15199 [push] Fix ASAN/UBSAN issues in... passed after 2 hr 32 min |
- Fixes a memory leak - Fixes an integer overflow caused by a dynamic shape - Fixes reshape to wrong type in LIT tests Signed-off-by: Rickert, Jonas <[email protected]>
- Fixes a memory leak - Fixes an integer overflow caused by a dynamic shape - Fixes reshape to wrong type in LIT tests Signed-off-by: Rickert, Jonas <[email protected]>
