Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

several improvements to Path::getAbsolutePath() #6542

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Jun 20, 2024

  • fixed result on Linux if parts discarded by .. do not exist
  • small adjustments to Windows implementation to match Linux one
  • throw from getAbsolutePath() if the given path does not exist

@firewave

This comment was marked as outdated.

@firewave

This comment was marked as outdated.

@firewave firewave marked this pull request as draft June 20, 2024 23:48
@firewave
Copy link
Collaborator Author

This now crashes in Cygwin:

Thread 1 "testrunner" received signal SIGABRT, Aborted.
0x00007ff87dd6d5e4 in ntdll!ZwWaitForSingleObject ()
   from /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll
(gdb) bt
#0  0x00007ff87dd6d5e4 in ntdll!ZwWaitForSingleObject ()
   from /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll
#1  0x00007ff87b5b1c4e in WaitForSingleObjectEx ()
   from /cygdrive/c/WINDOWS/System32/KERNELBASE.dll
#2  0x00007ff801d8ba60 in sigfillset () from /usr/bin/cygwin1.dll
#3  0x00007ff801d8793b in sched_getscheduler () from /usr/bin/cygwin1.dll
#4  0x00007ff801d87d11 in sched_getscheduler () from /usr/bin/cygwin1.dll
#5  0x00007ff801ed71d8 in cygwin1!abort () from /usr/bin/cygwin1.dll
#6  0x000000057b1a548e in cygstdc++-6!_ZN9__gnu_cxx27__verbose_terminate_handlerEv () from /usr/bin/cygstdc++-6.dll
#7  0x000000057b19bb86 in cygstdc++-6!_ZN10__cxxabiv111__terminateEPFvvE ()
   from /usr/bin/cygstdc++-6.dll
#8  0x000000057b29da73 in cygstdc++-6!.cxa_call_terminate ()
   from /usr/bin/cygstdc++-6.dll
#9  0x000000057b19befc in cygstdc++-6!_ZN10__cxxabiv129__pointer_to_member_type_infoD2Ev () from /usr/bin/cygstdc++-6.dll
#10 0x000000050caacbc5 in cyggcc_s-seh-1!_GCC_specific_handler ()
   from /usr/bin/cyggcc_s-seh-1.dll
#11 0x000000057b29e955 in cygstdc++-6!.gxx_personality_seh0 ()
   from /usr/bin/cygstdc++-6.dll
#12 0x00007ff87dd7292f in ntdll!.chkstk ()
   from /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll
#13 0x00007ff87dd22554 in ntdll!RtlRaiseException ()
   from /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll
#14 0x00007ff87dd7143e in ntdll!KiUserExceptionDispatcher ()
   from /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll
#15 0x00007ff87b5bba99 in RaiseException ()
   from /cygdrive/c/WINDOWS/System32/KERNELBASE.dll
#16 0x000000050caacca1 in cyggcc_s-seh-1!_Unwind_RaiseException ()
   from /usr/bin/cyggcc_s-seh-1.dll
#17 0x000000057b29e0fb in cygstdc++-6!.cxa_throw ()
   from /usr/bin/cygstdc++-6.dll
#18 0x000000057b29f793 in cygstdc++-6!_ZSt20__throw_system_errori ()
   from /usr/bin/cygstdc++-6.dll
#19 0x000000010107175b in std::mutex::lock (
    this=0x101603150 <realFilePathMap+48>)
    at /usr/lib/gcc/x86_64-pc-cygwin/11/include/c++/bits/std_mutex.h:104
#20 0x0000000100ff9dba in std::lock_guard<std::mutex>::lock_guard (
    this=0x7ffffb990, __m=...)
    at /usr/lib/gcc/x86_64-pc-cygwin/11/include/c++/bits/std_mutex.h:229
#21 0x000000010091770b in RealFileNameMap::getCacheEntry (
    this=0x101603120 <realFilePathMap>, path=..., returnPath=...)
    at externals/simplecpp/simplecpp.cpp:2362
#22 0x00000001004077a5 in realFilename (f=...)
    at externals/simplecpp/simplecpp.cpp:2430
#23 0x0000000100407f08 in simplecpp::simplifyPath (path=...)
    at externals/simplecpp/simplecpp.cpp:2565
#24 0x000000010066470a in Path::simplifyPath (originalPath=...)
    at lib/path.cpp:91
#25 0x0000000100665974 in Path::getAbsoluteFilePath (filePath=...)
    at lib/path.cpp:372
#26 0x000000010065061e in Library::load (this=0x7ffffc338,
    exename=0x57b2a25b8 <cygstdc++-6!_ZNSs4_Rep20_S_empty_rep_storageE+24> "",
    path=0x101362178 <__gnu_cxx::__default_lock_policy+68> "std.cfg",
    debug=false) at lib/library.cpp:139
#27 0x000000010041ad9f in TestFixture::SettingsBuilder::library (
    this=0x7ffffc1b0,
    lib=0x101362178 <__gnu_cxx::__default_lock_policy+68> "std.cfg")
    at test/fixture.cpp:458
#28 0x0000000100914fc5 in TestValueFlow::TestValueFlow (
    this=0x101662840 <(anonymous namespace)::instance_TestValueFlow>)
    at test/testvalueflow.cpp:42
#29 0x000000010042c6f1 in __static_initialization_and_destruction_0 (
    __initialize_p=1, __priority=65535) at test/testvalueflow.cpp:8566
#30 0x000000010042c72f in _GLOBAL__sub_I_testvalueflow.cpp(void) ()
    at test/testvalueflow.cpp:8566
#31 0x00007ff801d1664d in cygwin1!.main () from /usr/bin/cygwin1.dll
#32 0x000000010041e1d3 in main (argc=1, argv=0xa00001b30) at test/main.cpp:27

@firewave
Copy link
Collaborator Author

Looks like it is caused by the static initialization order. So this requires https://trac.cppcheck.net/ticket/12080 to be fixed first.

@firewave
Copy link
Collaborator Author

firewave commented Jul 1, 2024

We could also make the cache in simplecpp optional.

Nevertheless these caches are completely horrible and need to be implemented in a different way.

@firewave
Copy link
Collaborator Author

The crash is fixed because the test fixtures are no longer global objects which depend on the initialization order.

@firewave firewave changed the title small improvements to Path::getAbsolutePath() several improvements to Path::getAbsolutePath() Aug 15, 2024
if (absolute)
absolute_path = absolute;
free(absolute);
if (!filePath.empty() && !spath.empty() && absolute_path.empty() && !exists(spath))
throw std::runtime_error("path '" + filePath + "' does not exist");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sure we do not get unexpected results. This needs an integration test which triggers this though (I am hoping one of the existing ones will fail so I can derive from that).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only case where this could happen is in loadVisualStudioProperties() and that code looks quite shifty and need some proper testing. As Path::isAbsolute() also needs some work that will probably happen in that context.

lib/path.h Show resolved Hide resolved
@firewave firewave marked this pull request as ready for review September 18, 2024 10:19
@firewave
Copy link
Collaborator Author

We could also make the cache in simplecpp optional.

Nevertheless these caches are completely horrible and need to be implemented in a different way.

I filed danmar/simplecpp#361 upstream.

lib/path.cpp Outdated
std::string absolute_path;
#ifdef _WIN32
char absolute[_MAX_PATH];
if (_fullpath(absolute, filePath.c_str(), _MAX_PATH))
absolute_path = absolute;
if (absolute_path.back() == '\\')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a good chance that absolute_path is empty right? don't call back() in that case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would indeed be undefined: https://en.cppreference.com/w/cpp/string/basic_string/back - and we even report that. I wonder if that might exists in other parts of the code. We could probably be more aggressive about detecting that - maybe similar to a NULL dereference.

The _fullpath() call might need better errorhandling.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would indeed be undefined: https://en.cppreference.com/w/cpp/string/basic_string/back - and we even report that. I wonder if that might exists in other parts of the code. We could probably be more aggressive about detecting that - maybe similar to a NULL dereference.

This is not reported because it is in the _WIN32 code which we do not analyze in the CI. We should probably add analysis steps for files using those to the selfchecks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I also added a bunch of missing test cases and TODOs.

I also filed https://trac.cppcheck.net/ticket/13158 about enhancing the selfcheck.

@firewave firewave marked this pull request as draft September 29, 2024 00:00
@firewave firewave marked this pull request as ready for review September 30, 2024 16:06
lib/path.cpp Outdated
if (absolute)
absolute_path = absolute;
free(absolute);
if (!filePath.empty() && !spath.empty() && absolute_path.empty() && !exists(spath))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we know that filePath is not empty. so condition !filePath.empty() && is redundant.
I am not sure shouldn't we use || instead of && here. each of those conditions look bad?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though there is a preprocessor check that should be detected.

I will review the remaining conditions and add a comment about the intention.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed https://trac.cppcheck.net/ticket/13178 about detecting this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adjusted the check and added a comment.

@firewave firewave marked this pull request as draft October 4, 2024 12:24
@firewave firewave marked this pull request as ready for review November 14, 2024 21:29
@firewave firewave merged commit 640f82d into danmar:main Dec 4, 2024
60 checks passed
@firewave firewave deleted the abspath-test branch December 4, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants