This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Engine: fix crash with SEH __leave keyword
ClosedPublic

Authored by AbbasSabra on May 11 2021, 2:48 PM.

Diff Detail

Event Timeline

AbbasSabra created this revision.May 11 2021, 2:48 PM
AbbasSabra requested review of this revision.May 11 2021, 2:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 2:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
AbbasSabra edited the summary of this revision. (Show Details)May 11 2021, 2:49 PM

Please, add these reviewers for your upcoming [analyzer] patches.
Inline a couple of nits.
Nice to see some fixes for visual c++ stuff.

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
352

You should probably extend the ExprEngine.cpp:1312 in a similar fashion.

clang/test/Analysis/misc-ms-leave.cpp
13

If it crashed previously at this statement, please put there a // no-crash comment.

17

I guess you could simply use the clang_analyzer_warnIfReached() here.
https://clang.llvm.org/docs/analyzer/developer-docs/DebugChecks.html

Godin added a subscriber: Godin.May 12 2021, 3:14 AM

Updating D102280: [analyzer] Apply code review

AbbasSabra marked 2 inline comments as done.May 14 2021, 2:54 AM

Please, add these reviewers for your upcoming [analyzer] patches.

Thanks, I was planning to add reviewers after making sure that the pre-checks are green.

Nice to see some fixes for visual c++ stuff.

Thanks for the quick review!

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
352

isn't it already handled in ExprEngine.cpp:1239?

Do you have any good (mature, big enough) open-source projects for these msvc constructs?

clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
352

Good point.
At the other switch it states, that this leave statement is unsupported. I guess it will stay unsupported for a while because we don't model exceptions at all. Treat my previous comment as resolved.

clang/test/Analysis/misc-ms-leave.cpp
9

I'm curious to see if any statement after the __leave is executed. Could you place another warnIfReached there too?

Also, this file could contain all the ms try except constructs. That we might plan to support in the future.
So, a more generic file name would be more future-proof I guess.

Updating D102280: [analyzer] rename test file + make sure that statements after "__leave" are not reached

AbbasSabra marked an inline comment as done.May 14 2021, 6:03 AM

Do you have any good (mature, big enough) open-source projects for these msvc constructs?

https://github.com/microsoft/winfile and https://github.com/chakra-core/ChakraCore seem to be using SEH but not heavily.

Do you have any good (mature, big enough) open-source projects for these msvc constructs?

https://github.com/microsoft/winfile and https://github.com/chakra-core/ChakraCore seem to be using SEH but not heavily.

Well, and how can I build them on Linux xD
I tried mingw64, but failed :D

How do you use clang exactly?

Well, and how can I build them on Linux xD

Ah, I don't think they are meant to be built on Linux :D Especially if they rely on Window specific features like SEH. If they do they will disable these features on Linux build.

How do you use clang exactly?

I work on the C++ static analyzer at sonarsource and we use Clang front-end. We rely on CSA for some of our rules. So for these projects, we build them on windows. We extract the compilation command from the windows build and we map them to an equivalent clang command before running the analysis.

Is it a requirement for you to build them on Linux? Maybe you can try to build them on windows with clang-cl?

Is it a requirement for you to build them on Linux? Maybe you can try to build them on windows with clang-cl?

It is a hard requirement. I'm gonna think more about this in the far future I guess. Thanks for the insight though.

steakhal accepted this revision.May 17 2021, 1:49 AM

I think it's good to go. Thank you!

This revision is now accepted and ready to land.May 17 2021, 1:49 AM

I think it's good to go. Thank you!

Great! can you take care of merging it? I don't have accesss.

This revision was automatically updated to reflect the committed changes.