@steakhal

Losing git blame would have a signifficant impact in deed, however we always have the option to add the renaming commit to the .git-blame-ignore-revs.

Using this git feature I can see no more obstacles to implement the renaming. Moreover LLVM repo already has similar instances in .git-blame-ignore-revs.

To achieve that, we should probably have a better reason behind it besides simply renaming stuff.

Untill we start this process we will mix this up every time. So, I would start the renaming with the blame-ignore marking.

@NoQ , considered your suggestions about the case of recursive mutexes. Implemented this feature. Now this works like below:

recursive_mutex rm;
rm.lock();
rm.lock(); // no-warning
recursive_timed_mutex rtm;
rtm.lock();
rtm.lock(); // no-warning

Please look.

Just a ping. Let me, please, do smth with this revision.

Who can confirm if this is correct or somewhere it needs fixes? Here is a generated result of evalCast from the origin branch(before the patch):

void foo(int* x,  // &SymRegion{reg_$0<int * x>}
         int** y, // &SymRegion{reg_$1<int ** y>}
         int***z) // &SymRegion{reg_$2<int *** z>}
{
 (void*)x;    // &SymRegion{reg_$0<int * x>}
 (void*)y;    // &SymRegion{reg_$1<int ** y>}
 (void*)z;    // &SymRegion{reg_$2<int *** z>}
 (void**)x;   // &Element{SymRegion{reg_$0<int * x>},0 S64b,void *}
 (void**)y;   // &SymRegion{reg_$1<int ** y>}
 (void**)z;   // &SymRegion{reg_$2<int *** z>}
 (void***)x;  // &Element{SymRegion{reg_$0<int * x>},0 S64b,void **}
 (void***)y;  // &Element{SymRegion{reg_$1<int ** y>},0 S64b,void **}
 (void***)z;  // &SymRegion{reg_$2<int *** z>}
 (void****)x; // &Element{SymRegion{reg_$0<int * x>},0 S64b,void ***}
 (void****)y; // &Element{SymRegion{reg_$1<int ** y>},0 S64b,void ***}
 (void****)z; // &Element{SymRegion{reg_$2<int *** z>},0 S64b,void ***}
}

In D89055#2336709, @NoQ wrote:

Ugh, sorry, no, that's evalCast(). Like evalBinOp() etc. My bad. Can we also use evalCast()?

@OikawaKirie

Different from ProgramStateRef which is an alias to IntrusiveRefCntPtr, or StoreRef which is a wrapper object, an alias to a const SymExpr * makes no sense to me.

Yes. I omit this, because in such case we should go further and rename all those which are not real Ref to Ptr or smth that would emphasise that it's just a pointer alias, not a wrapper or another class.
That's why I prefered to change the name a little in favor of complex approach of renaming all the rest.

And this is also where I have been confused for a long while.

So have been I. The patch is called to make it more clear :)

Updated. Requalified this patch to non-functional changes.

@NoQ

The new code should obviously be restricted into evalCastFromLoc() because if it's a region it's a Loc.

The first I tryed was evalCastFromLoc(), but it turned out that SVal which binds to a pointer can be NonLoc as well through violation of pointing levels.
Look here:

void foo(int** p) { // here is a two-level pointer
   *(int*)p = 42; // pretend as a one-level pointer, dereference it and assign a number
   *p; // dereferencing once gives a `nonloc::ConcreteInt`, though it actually should be a one-level pointer aka Loc.
}

Actually it wasn't so hard as I thought :)
Would it be better to make this moving in a separate patch as [NFC]?

@NoQ

Can we collapse this function further towards this goal? Say, why not pump every region unconditionally through castRegion()? Does dispatchCast() use castRegion() internally - and if it does, why are there so many branches in this function?

I've made some investigations. castRegion() does not use inside dispatchCast(), it belongs to StoreManager. Moving it to SimpleSValBuilder will be hard at least for now, since it is quite complex, but we can try. What do you think. Should I go further or we can apply this patch as is?

Updated. Removed a new test file, moved the test to an existing file instead.

Updat patch due to suggestions and fixed formating.

It works for me. Well done!

In D87547#2322325, @mati865 wrote:

@ASDenysPetrov what CMake options do you use?
I'm unable to reproduce this error with Clang+LLD and GCC+Binutils from MSYS2.

@NoQ
BTW, what do you think we should do with D77062 and D88477 then?

@NoQ

Can we collapse this function further towards this goal? Say, why not pump every region unconditionally through castRegion()? Does dispatchCast() use castRegion() internally - and if it does, why are there so many branches in this function?

Hi, @mati865
Currently I got an error while building with GCC10 on Win10.

[2325/2656] Linking CXX shared library bin\libclang-cpp.dll
FAILED: bin/libclang-cpp.dll lib/libclang-cpp.dll.a
cmd.exe /C "cd . && D:\WORK\Utilities\MSYS2\mingw64\bin\c++.exe -Wa,-mbig-obj -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -fno-common -Woverloaded-virtual -fno-strict-aliasing  -O2   -shared -o bin\libclang-cpp.dll -Wl,--out-implib,lib\libclang-cpp.dll.a -Wl,--major-image-version,0,--minor-image-version,0 @tools\clang\tools\clang-shlib\CMakeFiles\clang-cpp.rsp  && cd ."
D:/WORK/Utilities/MSYS2/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: error: export ordinal too large: 66444
collect2.exe: error: ld returned 1 exit status
[2328/2656] Building CXX object tools/clang/tools/libclang/CMakeFiles/libclang.dir/CIndex.cpp.obj
ninja: build stopped: subcommand failed.

I've tryed this on two PCs. Could you, please, revise you solution. I'm not familiar with this part but this affects me (and I think other Win-people).

I've just uploaded a patch for solving this and related D77062. Welcome to review D89055.

@steakhal

Till then, I recommend you to follow my effort at D88477.

@ASDenysPetrov Do you still want to rework the API of the assumeZero?

@steakhal

If we get the value of **b, we get a NonLoc of type unsigned char.
The dump of **b confirms this: reg_$4<unsigned char Element{SymRegion{reg_$0<int * a>},0 S64b,unsigned char}>, which is a NonLoc in deed.

@baloghadamsoftware

Sorry, absolutely no competence.

It would be nice if someone had time to look at this. Thanks.

@NoQ

@steakhal

I would not accept this patch unless this investigation is done. However, I'm not inherently against this change.

Actually I've done the investigation. You can find it here https://reviews.llvm.org/D77062#1977613

Here is my five cents. I haven't done with the review yet. I'm gonna return to it a bit later.

One more notification. Please, somebody look at this patch.

@steakhal

the readability of the reports should be improved.

Absolutely agree. Let's do this in the next patches :)

Test fix.

Minor fixes.

Fixed some missed conditions. Added more tests.

The change LGTM! If it really can speed up performance or improve memory organization, let's load this patch.

In D87138#2256360, @martong wrote:

This seems to be a total non-functional-change. Please include [NFC] next time with a similar refactoring. Otherwise, Looks good to me, thanks!

A gentle notification :-)

Added timed functions support and tests for them.

Added shared semantics checks and correspondent tests.

Guys!
I've moved my work to PthreadLockChecker https://reviews.llvm.org/D85984. You are welcome to look at it.

Added support of:

• std::timed_mutex
• std::recursive_timed_mutex
• std::shared_mutex
• std::shared_timed_mutex

One more ping

LGTM!

@NoQ

That's completely different checker logic.

Added recursive_mutex support.

Hey, folk, welcome to https://reviews.llvm.org/D85984
I've moved the logic of this checker in PthreadLockChecker

@steakhal

If index1 and index2 has the same value, we should not be confident that the x == y holds.

Thanks! Now I see. Shame on me =)

@NoQ

You're on the right track but your checker repeats PthreadLockChecker word-by-word. Like, you can find answers to all your questions (eg., "how to use isLiveRegion?") by reading that checker. C++ functions aren't any different from C functions; that's an extremely minor difference that doesn't justify developing a new checker from scratch.

@NoQ

Umm, why don't you extend PthreadLockChecker instead?

@NoQ just one more ping. You accepted it and then I just revised it again. Could you, please, take a minute and look at it.
I'd close it with a great pleasure. It's been hanging too long.

Added recursive_mutex support.

Aded enum FuncIdKind to define function Ids.

@vsavchenko
Made changes due to your remarks. Thanks you.

Ping!

@vsavchenko:

However, I'm going to complain about commit messages again 😅 I would prefer having imperative mood in the message, something like "Refactor ..." or "Introduce minor refactoring..."

Changed title.

@vsavchenko

I again have to nitpick about the commit message, can you please change it to "Simplify ..."?

Ready! Sorry, I remember, you've already told me.

Changed naming due to LLVM rules.
I decided not to change names everywhere to leave the patch more readable.

One more :-)

@vsavchenko
OK, let it be. )

@vsavchenko
Thank you.
Despite of all of my nits, LGTM!