This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Improve FileSpec internal APIs and usage in preparation for adding caching of resolved/absolute.
ClosedPublic

Authored by clayborg on Jul 21 2022, 2:54 PM.

Details

Summary

The FileSpec APIs allow users to modify instance variables directly by getting a non const reference to the directory and filename instance variables. This makes it impossible to control all of the times the FileSpec object is modified so we can clear cached member variables like m_resolved and with an upcoming patch caching if the file is relative or absolute. This patch modifies the APIs of FileSpec so no one can modify the directory or filename instance variables directly by adding set accessors and by removing the get accessors that are non const.

Many clients were using FileSpec::GetCString(...) which returned a unique C string from a ConstString'ified version of the result of GetPath() which returned a std::string. This caused many locations to use this convenient function incorrectly and could cause many strings to be added to the constant string pool that didn't need to. Most clients were converted to using FileSpec::GetPath().c_str() when possible. Other clients were modified to use the newly renamed version of this function which returns an actualy ConstString:

ConstString FileSpec::GetPathAsConstString(bool denormalize = true) const;

This avoids the issue where people were getting an already uniqued "const char *" that came from a ConstString only to put the "const char *" back into a "ConstString" object. By returning the ConstString instead of a "const char *" clients can be more efficient with the result.

The patch:

  • Removes the non const GetDirectory() and GetFilename() get accessors
  • Adds set accessors to replace the above functions: SetDirectory() and SetFilename().
  • Adds ClearDirectory() and ClearFilename() to replace usage of the FileSpec::GetDirectory().Clear()/FileSpec::GetFilename().Clear() call sites
  • Fixed all incorrect usage of FileSpec::GetCString() to use FileSpec::GetPath().c_str() where appropriate, and updated other call sites that wanted a ConstString to use the newly returned ConstString appropriately and efficiently.

Diff Detail

Event Timeline

clayborg created this revision.Jul 21 2022, 2:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 2:54 PM
clayborg requested review of this revision.Jul 21 2022, 2:54 PM
Herald added a project: Restricted Project. · View Herald Transcript
JDevlieghere accepted this revision.Jul 21 2022, 5:10 PM

Thanks Greg, this is a much cleaner API. LGTM.

This revision is now accepted and ready to land.Jul 21 2022, 5:10 PM
clayborg edited the summary of this revision. (Show Details)Jul 22 2022, 10:09 AM
thakis added a subscriber: thakis.Jul 22 2022, 10:55 AM

This doesn't build, as far as I can tell: http://45.33.8.238/linux/82002/step_4.txt

Please take a look, and revert for now if it takes a while to fix.

I ran into this build break too - please try to fix or revert!

Looking at the build breakage now

Submitted a fix for the buildbots:

commit f959d815f4637890ebbacca379f1c38ab47e4e14 (HEAD -> main)
Author: Greg Clayton <gclayton@fb.com>
Date: Fri Jul 22 13:24:04 2022 -0700

Fix buildbot breakage after https://reviews.llvm.org/D130309.

Submitted a fix for the buildbots:

commit f959d815f4637890ebbacca379f1c38ab47e4e14 (HEAD -> main)
Author: Greg Clayton <gclayton@fb.com>
Date: Fri Jul 22 13:24:04 2022 -0700

Fix buildbot breakage after https://reviews.llvm.org/D130309.

Still broken for me:

../tools/lldb/source/Host/linux/HostInfoLinux.cpp: In static member function ‘static bool lldb_private::HostInfoLinux::ComputeSupportExeDirectory(lldb_private::FileSpec&)’:
../tools/lldb/source/Host/linux/HostInfoLinux.cpp:174:64: error: passing ‘const lldb_private::ConstString’ as ‘this’ argument discards qualifiers [-fpermissive]
  174 |   file_spec.GetDirectory() = GetProgramFileSpec().GetDirectory();
      |                                                                ^
In file included from ../tools/lldb/include/lldb/Utility/ArchSpec.h:13,
                 from ../tools/lldb/include/lldb/Host/HostInfoBase.h:12,
                 from ../tools/lldb/include/lldb/Host/posix/HostInfoPosix.h:12,
                 from ../tools/lldb/include/lldb/Host/linux/HostInfoLinux.h:12,
                 from ../tools/lldb/source/Host/linux/HostInfoLinux.cpp:9:
../tools/lldb/include/lldb/Utility/ConstString.h:40:7: note:   in call to ‘constexpr lldb_private::ConstString& lldb_private::ConstString::operator=(const lldb_private::ConstString&)’
   40 | class ConstString {
      |       ^~~~~~~~~~~

Another fix for linux:

commit 0bbce7a4c2d2bff622bdadd4323f93f5d90e6d24 (HEAD -> main, origin/main, origin/HEAD)
Author: Greg Clayton <gclayton@fb.com>
Date: Fri Jul 22 13:59:06 2022 -0700

Fix buildbot breakage after https://reviews.llvm.org/D130309.

Another fix for linux:

commit 0bbce7a4c2d2bff622bdadd4323f93f5d90e6d24 (HEAD -> main, origin/main, origin/HEAD)
Author: Greg Clayton <gclayton@fb.com>
Date: Fri Jul 22 13:59:06 2022 -0700

Fix buildbot breakage after https://reviews.llvm.org/D130309.

Thanks, now it finally built correctly for me!

Another fix for linux:

commit 0bbce7a4c2d2bff622bdadd4323f93f5d90e6d24 (HEAD -> main, origin/main, origin/HEAD)
Author: Greg Clayton <gclayton@fb.com>
Date: Fri Jul 22 13:59:06 2022 -0700

Fix buildbot breakage after https://reviews.llvm.org/D130309.

Thanks, now it finally built correctly for me!

Great! Thanks for the heads up on the breakage.

Still broken on windows for me: http://45.33.8.238/win/62911/step_4.txt

Seeing the same errors here.