This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Use virtual FS in processing config files
ClosedPublic

Authored by sepavloff on Aug 29 2022, 10:06 AM.

Details

Summary

Clang has support of virtual file system for the purpose of testing, but
treatment of config files did not use it. This change enables VFS in it
as well.

Diff Detail

Event Timeline

sepavloff created this revision.Aug 29 2022, 10:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2022, 10:06 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
sepavloff requested review of this revision.Aug 29 2022, 10:06 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 29 2022, 10:06 AM

This seems like the right thing in principle, but it's still pretty scary:

  • this is going to break anyone who's using VFS != RealFS together with config files, as we no longer search the old locations
  • the breakage is likely to be quiet/subtle, especially if it's e.g. an obscure flag being set in an arch-specific config file
  • the fixes for "part of the toolchain is missing from VFS" tend to need code changes in tools and tend to be hard to test

The thing the change has going for it is that I think config files and nontrivial VFS are both pretty rarely used, so maybe there are few people using them together.
(Google uses both, but having looked internally I don't believe we ever actually combine them).

So I think this should land, but it needs a release note as it's a non-obvious breaking change.
@MaskRay in case he has any thoughts about VFS use in driver in general.

llvm/lib/Support/CommandLine.cpp
1268

the old behavior was explicitly documented ("process' cwd is used instead", not FS's) so that documentation should be updated.

(I just talked to @kadircet about the patch that added that documentation, and we can't think of any reason that the old behavior is actually desirable)

1271

(This seems unidiomatic: consumeError(errorCodeToError(...)) is a no-op, we usually spell TODO as FIXME... but it matches the surrounding code, so best as-is I guess)

sepavloff updated this revision to Diff 458035.Sep 5 2022, 9:53 AM

Fix documentation

This seems like the right thing in principle, but it's still pretty scary:

  • this is going to break anyone who's using VFS != RealFS together with config files, as we no longer search the old locations
  • the breakage is likely to be quiet/subtle, especially if it's e.g. an obscure flag being set in an arch-specific config file
  • the fixes for "part of the toolchain is missing from VFS" tend to need code changes in tools and tend to be hard to test

The thing the change has going for it is that I think config files and nontrivial VFS are both pretty rarely used, so maybe there are few people using them together.
(Google uses both, but having looked internally I don't believe we ever actually combine them).

It seems that compatibility issue can appear if someone use VFS for all files but configuration one, which reside in real FS. It is inconvenient and hard to maintain, so probably nobody uses such combination, otherwise someone filed a bug or prepared a patch. Strictly speaking this is incorrect behavior because file system object must be used for all operations on files, according to documentation.

llvm/lib/Support/CommandLine.cpp
1268

You are right, I fixed that documentation.

It seems that compatibility issue can appear if someone use VFS for all files but configuration one, which reside in real FS. It is inconvenient and hard to maintain

I agree, the situation is overall better after this change.

probably nobody uses such combination, otherwise someone filed a bug or prepared a patch.

I can tell you from experience this doesn't always hold. It's unreasonable to expect users to know that the observed behavior is incorrect.

Strictly speaking this is incorrect behavior because file system object must be used for all operations on files, according to documentation.

I don't think this is a strong argument against documenting the change (is that what we're disagreeing about?)

  • what documentation? (E.g. Driver's constructor is undocumented)
  • actual behavior trumps documentation: a breaking change that brings behavior into line with docs is still breaking
  • it's not true in any case that all clang IO goes through the VFS, as a silly example consider plugin loading with -Xclang -load.
  • in practice many users who rely on these details work it out by trial and error
sepavloff updated this revision to Diff 458205.Sep 6 2022, 10:20 AM

Add a release note

Strictly speaking this is incorrect behavior because file system object must be used for all operations on files, according to documentation.

I don't think this is a strong argument against documenting the change (is that what we're disagreeing about?)

No problem to add documentation. I added a release note, if you mean that.

  • in practice many users who rely on these details work it out by trial and error

Absolutely!

sammccall accepted this revision.Sep 7 2022, 7:42 AM

Thanks!

clang/docs/ReleaseNotes.rst
87–88

Because this is changing behavior that people might be relying on, I think it's important to call out precisely what the change is. Suggested a tweak.

This revision is now accepted and ready to land.Sep 7 2022, 7:42 AM
This revision was landed with ongoing or failed builds.Sep 9 2022, 2:29 AM
This revision was automatically updated to reflect the committed changes.