Page MenuHomePhabricator

Replace `T(x)` with `reinterpret_cast<T>(x)` everywhere it means reinterpret_cast. No functional change
AcceptedPublic

Authored by Quuxplusone on Mar 22 2020, 12:57 PM.

Details

Summary

I wrote a Clang warning [not pictured] to diagnose any use of T(x) which was not equivalent to static_cast<T>(x). Then I built Clang with that warning turned on. Here are all of the instances of "unsafe" T(x) that it identified in the codebase. Every single one is a cast from some pointer type to uintptr_t.

N.B.: The code in llvm/IR/SymbolTableListTraits.h looks absolutely crazy, and this will be the first time it's been touched in over a decade.

If this refactor is acceptable, I'll need someone to land it for me, as I don't have permissions.

Diff Detail

Event Timeline

Quuxplusone created this revision.Mar 22 2020, 12:57 PM

Passing-by remark:

I wrote a Clang warning [not pictured] to diagnose any use of T(x) which was not equivalent to static_cast<T>(x).

I'm not sure whether or not this will pass the bar for a clang diagnostic,
but that does sound like a good clang-tidy readability check.

I also have a bit of pet peeve with T(x) (i.e. including those that should be static_cast<>),
so if you do go with clang-tidy direction, it may be good to generalize it to handle all T(x),
but diagnose only those cast types that are specified in the config.

@Quuxplusone - did you see that there are several clang-format warnings reported against these changes? Please could you fix them.

@jhenderson: Updated! clang-format's suggested linebreaks look horrendous to me, but if this makes that buildbot happy, okay... I guess the moral of the story is "deeply nested expressions that also use lots of reinterpret_casts, should be avoided," and I can't argue with that. ;)

Nice. Does LibreOffice have anything (either in clang-tidy or in a paper guideline) against T(x)-style casts? E.g.

struct PB {};
struct D : private PB {};
using FloatRef = float&;
using IntPtr = int*;
using PBPtr = PB*;
int i = 42;
const int *cpi = &i;
D *pd = nullptr;
float& f = FloatRef(i);  // reinterpret_cast
int *pi = IntPtr(cpi);  // const_cast
PB *pb = PBPtr(pd);  // cast to private base

Nice. Does LibreOffice have anything (either in clang-tidy or in a paper guideline) against T(x)-style casts? E.g.

No, we don't have very many of those in our codebase, so we have left them alone.
Our plugin is designed to convert c-style casts to modern C++ casts.

rsmith accepted this revision.EditedMar 23 2020, 12:12 PM

Changes LGTM on the Clang side.

Passing-by remark:

I wrote a Clang warning [not pictured] to diagnose any use of T(x) which was not equivalent to static_cast<T>(x).

I'm not sure whether or not this will pass the bar for a clang diagnostic

I'd like to try it out on a larger codebase, but it sounds at least potentially good to me. There's a simple syntactic workaround (use (T)x instead of T(x)), and there's a high likelihood that the code doesn't mean what the programmer intended.

Does the warning catch the cases where the code is equivalent to static_cast<T>(x) except that it ignores access? That seems like a really good thing to warn on regardless of whether we warn on the general case.

This revision is now accepted and ready to land.Mar 23 2020, 12:12 PM

Passing-by remark:

I wrote a Clang warning [not pictured] to diagnose any use of T(x) which was not equivalent to static_cast<T>(x).

I'm not sure whether or not this will pass the bar for a clang diagnostic

I'd like to try it out on a larger codebase, but it sounds at least potentially good to me. There's a simple syntactic workaround (use (T)x instead of T(x)), and there's a high likelihood that the code doesn't mean what the programmer intended.

Does the warning catch the cases where the code is equivalent to static_cast<T>(x) except that it ignores access? That seems like a really good thing to warn on regardless of whether we warn on the general case.

Yes, I verified with some simple test cases that it catches the equivalents of (1) reinterpret_cast, (2) const_cast, (3) const_cast plus static_cast, (4) upcast to private base, and (5) downcast to private base.
However, the thing I wrote is 100% not ready for prime time. It basically just inserts a call to TryStaticCast from inside CheckCXXCStyleCast. Unfortunately TryStaticCast is one of these functions that Clang is full of, where it both "checks if X is possible" and also "does X" and also "prints error messages to stdout," all from within the same function. So my patch ends up being morally equivalent to just treating T(x) as static_cast<T>(x); it emits some errors that shouldn't be there if it's supposed to just warn, and I have no idea how to shut up those errors except by completely refactoring TryXXXXCast. (E.g., it could take an extra parameter Mode that indicated whether to actually do the conversion, or just report whether it was possible, or emit notes about why it was impossible.) I wrestled unsuccessfully with similar issues when I did -Wreturn-std-move.

I 100% want to see someone like you tackle this issue, but take my word for it, you don't want to start with the code I wrote the other day. :)

[...] take my word for it, you don't want to start with the code I wrote the other day. :)

Understood, thanks :) (FWIW, I had a stab at implementing essentially this last year and hit exactly the issues you highlighted.)

jhenderson accepted this revision.Mar 25 2020, 1:29 AM

LGTM, with nit, with regards to the llvm-readobj and libObject parts. I haven't looked at the other bits though.

llvm/lib/CodeGen/AsmPrinter/OcamlGCPrinter.cpp
150

Nit: I think you can fold these two string literals together. Not sure why they were separate before.

Quuxplusone marked 2 inline comments as done.Mar 25 2020, 12:12 PM

Btw, @jhenderson and @rsmith, if you wanted to land your own files' pieces of this patch while waiting on whoever-it-is to approve the pieces in other files, that'd be cool. As I mentioned initially, I don't have the access necessary to land any part of it myself.

llvm/lib/CodeGen/AsmPrinter/OcamlGCPrinter.cpp
150

I think the current code is intentionally parallel to what's on line 162, so I'd prefer to leave it alone and let whoever maintains it reflow it if they want.

Btw, @jhenderson and @rsmith, if you wanted to land your own files' pieces of this patch while waiting on whoever-it-is to approve the pieces in other files, that'd be cool. As I mentioned initially, I don't have the access necessary to land any part of it myself.

From my point of view, I don't see any particular rush to land this, so let's just keep it all as one, and wait for the other bits to be reviewed.

sberg added a subscriber: sberg.Mar 27 2020, 1:32 AM

Nice. Does LibreOffice have anything (either in clang-tidy or in a paper guideline) against T(x)-style casts? E.g.

No, we don't have very many of those in our codebase, so we have left them alone.
Our plugin is designed to convert c-style casts to modern C++ casts.

Heh, I've meanwhile improved LibreOffice's cstylecast check now (after I'd read only the first few initial comments here), to also flag function-style casts that should be const_/reinterpret_cast, and it indeed only found a handful of cases that would better be reinterpret_cast (https://gerrit.libreoffice.org/plugins/gitiles/core/+/1ebeacb20ad0165e399629fcfd7795ad0da3edf8%5E!/ "Extend loplugin:cstylecast to certain function-style casts"). Though in two of those places it conveniently unearthed code that should rather do without any kind of casting (addressed with follow-up https://gerrit.libreoffice.org/plugins/gitiles/core/+/741d30b5e1b0dcdbafb300ed7c7ad46756ffd946%5E!/ "Simplify pointer equality comparison" and https://gerrit.libreoffice.org/plugins/gitiles/core/+/e3196f3dddad6e7825db3b35e8196be35b466fd9%5E!/ "Fix pointer equality comparision").

I am guessing @DiggerLin was pinged with regards to XCOFFObjectFile.cpp. @jhenderson already reviewed and approved the changes to that file (it falls under libObject).

Ah. Then I'm not sure who else to ping, or even which pieces remain in need of review.
CODE_OWNERS.TXT isn't being very helpful here...
@kparzysz for HexagonCommonGEP.cpp ("Hexagon backend")?
@Bigcheese for Binary.h ("Object")?

All these changes are rather NFCI, i'm not sure each one needs to be reviewed.

Bigcheese accepted this revision.Apr 3 2020, 2:12 PM

LGTM for llvm/{lib,include/llvm}/Object/* and llvm/tools/llvm-readobj/*.

I'm not sure which part I'm supposed to look at, but the changes LGTM.