Clang-tidy invalid suggestion: llvm-twine-local

[joker-eph]

With clang-tidy built at current head and applying it on MLIR:

$ clang-tidy -p build mlir/lib/Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp --checks=-*,llvm-twine-local -fix -fix-errors

diff --git a/mlir/lib/Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp b/mlir/lib/Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp
index 453b9bc1b58b..78402e5ea896 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp
@@ -416,7 +416,7 @@ llvm::LaunchKernel::createKernelLaunch(mlir::gpu::LaunchFuncOp op,
 
   // Load the kernel function.
   StringRef moduleName = op.getKernelModuleName().getValue();
-  Twine moduleIdentifier = getModuleIdentifier(moduleName);
+  std::string moduleIdentifier = (getModuleIdentifier(moduleName)).str();
   Value *modulePtr = module.getGlobalVariable(moduleIdentifier.str(), true);
   if (!modulePtr)
     return op.emitError() << "Couldn't find the binary: " << moduleIdentifier;

mlir/lib/Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp:420:64: error: no member named 'str' in 'std::basic_string<char>'
  420 |   Value *modulePtr = module.getGlobalVariable(moduleIdentifier.str(), true);
      |                                               ~~~~~~~~~~~~~~~~ ^
1 error generated.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Mehdi Amini (joker-eph)

With clang-tidy built at current head and applying it on MLIR:

$ clang-tidy -p build mlir/lib/Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp --checks=-*,llvm-twine-local -fix -fix-errors

diff --git a/mlir/lib/Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp b/mlir/lib/Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp
index 453b9bc1b58b..78402e5ea896 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp
@@ -416,7 +416,7 @@ llvm::LaunchKernel::createKernelLaunch(mlir::gpu::LaunchFuncOp op,
 
   // Load the kernel function.
   StringRef moduleName = op.getKernelModuleName().getValue();
-  Twine moduleIdentifier = getModuleIdentifier(moduleName);
+  std::string moduleIdentifier = (getModuleIdentifier(moduleName)).str();
   Value *modulePtr = module.getGlobalVariable(moduleIdentifier.str(), true);
   if (!modulePtr)
     return op.emitError() << "Couldn't find the binary: " << moduleIdentifier;

mlir/lib/Target/LLVMIR/Dialect/GPU/SelectObjectAttr.cpp:420:64: error: no member named 'str' in 'std::basic_string<char>'
  420 |   Value *modulePtr = module.getGlobalVariable(moduleIdentifier.str(), true);
      |                                               ~~~~~~~~~~~~~~~~ ^
1 error generated.

[vbvictor]

I believe that llvm::Twine has many (if not all) methods that are incompatible with std::string so the check can't in theory make follow-up code fixes. E.g. what to do with isValid, isEmpty, etc...

So this can't be reasonably fixed.

[vbvictor]

Looking at it again, the best I can do is to convert fix-its to "fix-its notes" which means they will only be applied with -fix-notes flag.
This tells the developer that fix-its are not expected to be always correct or cover 100% of use-case

[joker-eph]

Can we check the way the Twine is used and validate if the replacement is valid by whitelisting the method that exists on the std::string class as well?

[vbvictor]

Can we check the way the Twine is used and validate if the replacement is valid by whitelisting the method that exists on the std::string class as well?

We probably can, but at cost unless there are already utility functions for it.
However, I personally don't see any overall net benefit (maybe someone else would).
Imagine 2 scenarios when we implemented this feature vs not implemented.
The check found a local Twine, the actions could be:

1. If we didn't implement, the check would transform class -> developer gets compiler error (5-10sec for rebuild) -> developer fixes all incompatibility by hand.
2. If we implemented feature, the check wouldn't transform class -> developer write std::string and fixes all incompatibility by hand.

So the benefit is only in skipping "gets compiler error" part which shouldn't take long (maybe I'm wrong here) and probably removing some frustration with fix-its not working (this should be fixed by fix-notes). But I don't see any big time-saver with this implemented.

[joker-eph]

It seems that you're assuming there is a user with a local build and an IDE using it in their development flow.
The context of the tickets I filed was an intent expressed on Discourse of using clang-tidy in the future for in pre-commit CI, which I would consider only if fix-its are not invalid. So the value is in the automation that can be built around this.

[vbvictor]

It seems that you're assuming there is a user with a local build and an IDE using it in their development flow.

Yes, I assume it by default.
In case of CI, maybe it's better to just disable fix-its for this check. One could ask why std::string is the right substitution for Twine? Maybe StringRef or some other class is better.

P.S. I didn't find in https://llvm.org/docs/CodingStandards.html justification to not use local Twine variables. In original commit 190e2cf, it's even said that Technically it is safe to have a Twine with >=2 components in a variable but I don't think it is a good pattern to follow.. So the check does not reflect LLVM Coding Guidelines, thus could be disabled.

[joker-eph]

Since Twine is a LLVM specific thing, it's a bit weird to have a check that exists, but that we don't use because it's not implementing a suggestion that we agree with: should this check be removed then?

[vbvictor]

Since Twine is a LLVM specific thing, it's a bit weird to have a check that exists, but that we don't use because it's not implementing a suggestion that we agree with: should this check be removed then?

We can't say for sure that "we don't use". Before removing it, we should gather opinions from developers across the project.
If we introduce clang-tidy CI, then we would have more interested parties to share opinions and make a decision.

[HerrCai0907]

I think the reason about this check is memory leak.
llvm::Twine does not hold any ownership of the string. Abuse it may cause use after free.
IMO, replace llvm::Twine with std::string is fine. But after fixing, I think we should fix all position which use this variable manually.
For this case, It is ok to return Twine because moduleName and string literal exist inside the call.

static Twine getModuleIdentifier(StringRef moduleName) {
  return moduleName + "_module";
}

[HerrCai0907]

Ahh, actually it could be a problem if you call it like

Twine result = getModuleIdentifier(std::string{"abc") + "def");

The StringRef point to the template std::string and will be free after call. But the Twine still points to this position.

I prefer to let it return std::string to avoid this misuse.