[mlir][td] check-use-after-free.mlir failing (AppleSillcon + shared libs)

[banach-space]

Hi,

check-use-after-free.mlir fails for for me on AppleSilicon (checked on M3 and M4) with:

• -DBUILD_SHARED_LIBS=On

(static libs build is fine).

I've tracked it down to the TD logic incorrectly assuming that at runtime there will be only one instance of DerivedResource. That assumption is effectively made here:

llvm-project/mlir/include/mlir/Interfaces/SideEffectInterfaceBase.td

Line 132 in 78c96aa

return it.getSymbolRef() != value;

I actually end up with two instances of that class at runtime, both are instantiations of:

mlir::SideEffects::Resource::Base<mlir::transform::TransformMappingResource, mlir::SideEffects::Resource>:

• one instance "lives" in libMLIRTransformDialectInterfaces.dylib,
• another instances "lives" in libMLIRTransformDialectTransforms.dylib.

QUESITON:

Should I update getEffectsOnResource to compare ResourceID instead?

return it.getResource()->getResourceID() != resource->getResourceID();

llvm-project/mlir/include/mlir/Interfaces/SideEffectInterfaces.h

Line 108 in 78c96aa

TypeID getResourceID() const { return id; }

Alternatively, we could explore ways to ensure that there is only one instance across all shared objects, but I’m not sure how best to achieve that.

@ftynse, could you take a look when you get a chance? "git log" wasn’t very helpful here (this logic depends on quite a few components), and you’re my go-to TD expert. 🙂

—Andrzej

[llvmbot]

@llvm/issue-subscribers-mlir

Author: Andrzej Warzyński (banach-space)

Hi,

check-use-after-free.mlir fails for for me on AppleSilicon (checked on M3 and M4) with:

• -DBUILD_SHARED_LIBS=On

(static libs build is fine).

I've tracked it down to the TD logic incorrectly assuming that at runtime there will be only one instance of DerivedResource. That assumption is effectively made here:

llvm-project/mlir/include/mlir/Interfaces/SideEffectInterfaceBase.td

Line 132 in 78c96aa

return it.getSymbolRef() != value;

I actually end up with two instances of that class at runtime, both are instantiations of:

mlir::SideEffects::Resource::Base<mlir::transform::TransformMappingResource, mlir::SideEffects::Resource>:

• one instance "lives" in libMLIRTransformDialectInterfaces.dylib,
• another instances "lives" in libMLIRTransformDialectTransforms.dylib.

QUESITON:

Should I update getEffectsOnResource to compare ResourceID instead?

return it.getResource()->getResourceID() != resource->getResourceID();

llvm-project/mlir/include/mlir/Interfaces/SideEffectInterfaces.h

Line 108 in 78c96aa

TypeID getResourceID() const { return id; }

Alternatively, we could explore ways to ensure that there is only one instance across all shared objects, but I’m not sure how best to achieve that.

@ftynse, could you take a look when you get a chance? "git log" wasn’t very helpful here (this logic depends on quite a few components), and you’re my go-to TD expert. 🙂

—Andrzej

[ftynse]

SideEffectInterfaceBase.td is core IR, so this may be me misusing it in TD or a broader issue. Could you check why we don't get a similar problem with the DefaultResource regular memory effects are associated with?

[banach-space]

Could you check why we don't get a similar problem with the DefaultResource regular memory effects are associated with?

It’s a bit tricky to trigger. However, after comparing the implementation, this appears to be a broader issue, with TD and this particular test simply exposing it.

Btw, click to expand to see my repro ...

func.func @use_after_free_branching_control_flow() {
  // expected-note @below {{allocated here}}
  %0 = transform.test_produce_self_handle_or_forward_operand : () -> !transform.any_op
  transform.test_transform_op_with_regions {
    "transform.test_branching_transform_op_terminator"() : () -> ()
  },
  {
  ^bb0:
    "transform.test_branching_transform_op_terminator"()[^bb1, ^bb2] : () -> ()
  ^bb1:
    // expected-note @below {{freed here}}
    transform.test_consume_operand_of_op_kind_or_fail %0, "transform.test_produce_self_handle_or_forward_operand" : !transform.any_op
    "transform.test_branching_transform_op_terminator"()[^bb3] : () -> ()
  ^bb2:
    "transform.test_branching_transform_op_terminator"()[^bb3] : () -> ()
  ^bb3:
    // expected-warning @below {{operand #0 may be used after free}}
    transform.sequence %0 : !transform.any_op failures(propagate) {
    ^bb0(%arg0: !transform.any_op):
    }
    "transform.test_branching_transform_op_terminator"() : () -> ()
  }
  return
}

FURTHER ANALYSIS

I compared the shared objects on Linux (.so) and macOS (.dylib), looking for:

• Definition of DerivedResource::instance.
• How it is treated in shared objects

MacOS

# De-mangle the name that I am searching for below:
$ c++filt __ZZN4mlir11SideEffects8Resource4BaseINS_9transform24TransformMappingResourceES1_E3getEvE8instance
mlir::SideEffects::Resource::Base<mlir::transform::TransformMappingResource, mlir::SideEffects::Resource>::get()::instance

$ cd <llvm-build-dir>
$ for file in lib/*.dylib; do llvm-nm $file | grep __ZZN4mlir11SideEffects8Resource4BaseINS_9transform24TransformMappingResourceES1_E3getEvE8instance && echo $file; done
00000000000b8090 d __ZZN4mlir11SideEffects8Resource4BaseINS_9transform24TransformMappingResourceES1_E3getEvE8instance
lib/libMLIRTestTransformDialect.dylib
0000000000040070 d __ZZN4mlir11SideEffects8Resource4BaseINS_9transform24TransformMappingResourceES1_E3getEvE8instance
lib/libMLIRTransformDialectInterfaces.dylib
0000000000028030 d __ZZN4mlir11SideEffects8Resource4BaseINS_9transform24TransformMappingResourceES1_E3getEvE8instance
lib/libMLIRTransformDialectTransforms.dylib

• The instance symbol is marked d (local data symbol) in macOS .dylib files.
• This means each shared object gets its own local copy of DerivedResource::instance.

Wiki docs: https://en.wikipedia.org/wiki/Nm_(Unix). From Linux man pages:

"D"
"d" The symbol is in the initialized data section

Linux

# De-mangle the name that I am searching for below:
$ c++filt _ZZN4mlir11SideEffects8Resource4BaseINS_9transform24TransformMappingResourceES1_E3getEvE8instance
mlir::SideEffects::Resource::Base<mlir::transform::TransformMappingResource, mlir::SideEffects::Resource>::get()::instance

$ cd <llvm-build-dir>
$ for file in lib/*.so; do  nm $file | grep _ZZN4mlir11SideEffects8Resource4BaseINS_9transform24TransformMappingResourceES1_E3getEvE8instance && echo $file; done
0000000000128d40 V _ZZN4mlir11SideEffects8Resource4BaseINS_9transform24TransformMappingResourceES1_E3getEvE8instance
lib/libMLIRTestTransformDialect.so
000000000007d9e8 V _ZZN4mlir11SideEffects8Resource4BaseINS_9transform24TransformMappingResourceES1_E3getEvE8instance
lib/libMLIRTransformDialectInterfaces.so
00000000000575f8 V _ZZN4mlir11SideEffects8Resource4BaseINS_9transform24TransformMappingResourceES1_E3getEvE8instance
lib/libMLIRTransformDialectTransforms.so

• The instance symbol is marked V (weak object symbol).
• Weak symbols allow multiple definitions across shared objects but prefer a single unique instance if available.

Fom Linux man pages on V:

"V"
"v" The symbol is a weak object.  When a weak defined symbol is linked with a normal defined symbol, the normal defined symbol is used with no error.  When a weak undefined symbol is linked and the
   symbol is not defined, the value of the weak symbol becomes zero with no error.  On some systems, uppercase indicates that a default value has been specified.

CONCLUSION

• On Linux, instance is treated as a weak symbol (V), meaning all shared objects share one instance.
• On macOS, instance is not weak (d, local data), meaning each shared object gets its own separate instance.

This breaks pointer comparison:

effectOne.getResource() != effectOne.getResource();

POSSIBLE FIXES

FIX 1: We could compare "kind"s:

effectOne.getResource()->getResourceID() != effectTwo.getResource()->getResourceID();

FIX 2: If pointer comparison must be used, we can explicitly instantiate DerivedResource::instance in one .cpp file. But that feels fragile (we'd need to be careful not violate ODR).

FIX 3: Perhaps we are just missing some linker flag somehwere?

[ftynse]

FIX 2: If pointer comparison must be used, we can explicitly instantiate DerivedResource::instance in one .cpp file. But that feels fragile (we'd need to be careful not violate ODR).

I suspect DerivedResource::instance predates the current form of TypeID and we should just use the latter more pervasively.