Page MenuHomePhabricator

[flang] Refine output file generation
ClosedPublic

Authored by awarzynski on Aug 19 2021, 10:17 AM.

Details

Summary

This patch cleans-up the file generation code in Flang's frontend driver. It improves the layering between CompilerInstance::CreateDefaultOutputFile, CompilerInstance::CreateOutputFile and their various clients.

  • Rename CreateOutputFile as CreateOutputFileImpl and make it private. This method is an implementation detail.
  • Instead of passing an std::error_code out parameter into CreateOutputFileImpl, have it return Expected<>. This is a bit shorter and idiomatic LLVM.
  • Make CreateDefaultOutputFile (which calls CreateOutputFileImpl) issue an error when file creation fails. The error code from CreateOutputFileImpl is used to generate a meaningful diagnostic message.
  • Remove error reporting from PrintPreprocessedAction::ExecuteAction. This is only for cases when output file generation fails. This is handled in CreateDefaultOutputFile instead (see the previous point).
  • Inline AddOutputFile into its only caller, CreateDefaultOutputFile.
  • Switch from lvm::buffer_ostream to llvm::buffer_unique_ostream> for non-seekable output streams. This simplifies the logic in the driver and was introduced for this very reason in [1]
  • Moke sure that the diagnostics from the prescanner when running -E (PrintPreprocessedAction::ExecuteAction) are printed before the actual output is generated.
  • Update comments, add test.

[1] https://reviews.llvm.org/D93260

Diff Detail

Event Timeline

awarzynski created this revision.Aug 19 2021, 10:17 AM
awarzynski requested review of this revision.Aug 19 2021, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2021, 10:17 AM
awarzynski edited the summary of this revision. (Show Details)Aug 19 2021, 10:24 AM
awarzynski added a project: Restricted Project.
This revision is now accepted and ready to land.Aug 19 2021, 12:54 PM
This revision was landed with ongoing or failed builds.Aug 19 2021, 11:47 PM
This revision was automatically updated to reflect the committed changes.
awarzynski reopened this revision.Aug 20 2021, 2:17 AM
This revision is now accepted and ready to land.Aug 20 2021, 2:17 AM
awarzynski added a subscriber: Meinersbur.EditedAug 20 2021, 2:22 AM

I reverted this as our Windows buildbot was failing [1]. I'm not sure what went wrong, I used %errc_ENOENT in output-paths.f90, so this shouldn't really happen:

C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\build\tools\flang\test\Driver\Output\output-paths.f90.tmp:1:1: note: with "MSG" equal to "No such file or directory"
error: unable to open output file 'C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\build\tools\flang\test\Driver\Output\output-paths.f90.tmp.doesnotexist/somename': 'no such file or directory'

Basically, it looks that %errc_ENOENT is expanded to "No such file or directory" rather than "no such file or directory".

@Meinersbur , would you be able to take a look? Thank you!

[1] https://lab.llvm.org/buildbot/#/builders/172/builds/2077

DavidSpickett added a subscriber: DavidSpickett.EditedAug 20 2021, 3:22 AM

Something I found on windows on Arm is that cmake was trying to run every try_compile or try_run in debug mode but we didn't have the debug dlls on the path because we were doing a release llvm build. llvm uses a try_run to work out what those generic file errors should be, and it always fails for that reason and so uses the default unix style errors.

Might be worth looking at. You can set -DCMAKE_TRY_COMPILE_CONFIGURATION=Release if that is the case. I found a similar cmake issue https://gitlab.kitware.com/cmake/cmake/-/issues/16553 but haven't looked into it further yet.

See llvm/cmake/modules/GetErrcMessages.cmake

Meinersbur added a comment.EditedAug 20 2021, 1:02 PM

I logged into the worker to see the value it is using:

config.errc_messages = "no such file or directory;is a directory;invalid argument;permission denied"

However, this is LLVM's lit.site.cfg.py, flang does not have an equivalent errc_messages. This causes lit (llvm-project\llvm\utils\lit\lit\llvm\config.py) to fall back to Python's error messages:

# Python's strerror may not supply the same message
# as C++ std::error_code. One example of such a platform is
# Visual Studio. errc_messages may be supplied which contains the error
# messages for ENOENT, EISDIR, EINVAL and EACCES as a semi colon
# separated string. LLVM testsuites can use get_errc_messages in cmake
# to automatically get the messages and pass them into lit.
errc_messages = getattr(self.config, 'errc_messages', '')
if len(errc_messages) != 0:
    (errc_enoent, errc_eisdir,
     errc_einval, errc_eacces) = errc_messages.split(';')
    self.config.substitutions.append(
        ('%errc_ENOENT', '\'' + errc_enoent + '\''))
     ...
else:
    self.config.substitutions.append(
        ('%errc_ENOENT', '\'' + os.strerror(errno.ENOENT) + '\''))
     ...

That is, I suggest to add the following line

config.errc_messages = "@LLVM_LIT_ERRC_MESSAGES@"

into flang's lit.site.cfg.py.in to fix this. Also ensure that LLVM_LIT_ERRC_MESSAGES is populated in standalone builds, like clang does.

Meinersbur added a comment.EditedAug 20 2021, 1:16 PM

@DavidSpickett: Look like the appropriate ucrtbased.dll (debug version of ucrtbase.dll) is not installed into system folders. Also see https://stackoverflow.com/questions/33743493/why-visual-studio-2015-cant-run-exe-file-ucrtbased-dll. The try_run calls should not need paths to any LLVM dll.

Thank you @Meinersbur , this is incredibly helpful and much appreciated! I really had no clue where to look in this case. I'll update this patch in a few minutes.

Also ensure that LLVM_LIT_ERRC_MESSAGES is populated in standalone builds, like clang does.

I think that we are missing some logic surrounding test configuration for standalone builds, so I'm not sure _where_ it should go. Could you take a look?

Add config.errc_messages="@LLVM_LIT_ERRC_MESSAGES@"

Also ensure that LLVM_LIT_ERRC_MESSAGES is populated in standalone builds, like clang does.

I think that we are missing some logic surrounding test configuration for standalone builds, so I'm not sure _where_ it should go. Could you take a look?

From clang's CMakeLists.txt:

if( CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR )
  ...
  include(GetErrcMessages)
  ...
  get_errc_messages(LLVM_LIT_ERRC_MESSAGES)

Add missing include, fix clang-tidy warning

The pre-commit CI is now passing and so I will merge this shortly (I will be on a break for 2 weeks and would really prefer for this to be complete before I leave). Thank you all for helping with this!

This revision was automatically updated to reflect the committed changes.

The x64 windows pre-check bot is failing on this test, see:

failed build


Failed Tests (1):

Flang :: Driver/output-paths.f90

The x64 windows pre-check bot is failing on this test, see:

failed build

Hi @yaron.keren , thank you for pointing this out. This test was failing in the original version of the patch, which then got reverted, updated/fixed and re-committed. I see that you have already merged your patch, so I guess that all is good? If there are any outstanding issues, I will try to address them ASAP. Sadly, I can't check the buildbots to verify this as they are offline ATM.

Oh, and apologies for the delay, I was AFK for a couple of weeks.

Yes the test had passed since then, so all OK.