Page MenuHomePhabricator

[clang] stop baremetal driver to append .a to lib
ClosedPublic

Authored by christof on Feb 3 2020, 9:03 AM.

Details

Summary

When the clang baremetal driver selects the rt.builtins static library it prefix with "-l" and appends ".a". The result is a nonsense option which lld refuses to accept.

Diff Detail

Event Timeline

christof created this revision.Feb 3 2020, 9:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2020, 9:03 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I think this is the right thing to do. According to https://sourceware.org/binutils/docs/ld/Options.html#Options

Add the archive or object file specified by namespec to the list of files to link. This option may be used any number of times. If namespec is of the form :filename, ld will search the library path for a file called filename, otherwise it will search the library path for a file called libnamespec.a.

An argument could be made that LLD could be a bit cleverer and only add the .a when it doesn't exist, although that would fail in the contrived case that someone had explicitly named their library lib name.a.a however it would be good to fix this in clang so that older versions of LLD will work.

Please could you add a test to clang/test/Driver/arm-compiler-rt.c for the arm-none-eabi target.

+1, looks good with a test.

christof updated this revision to Diff 243868.Feb 11 2020, 7:39 AM

With updated tests

Just noticed the comment from Peter asking for clang/test/Driver/arm-compiler-rt.c, I'll add that in a moment.

psmith accepted this revision.Feb 11 2020, 8:12 AM
psmith added a subscriber: psmith.

Thanks for the update, looks good to me. The BareMetal driver tests are better than the location I suggested.

This revision is now accepted and ready to land.Feb 11 2020, 8:12 AM
christof updated this revision to Diff 243896.Feb 11 2020, 9:03 AM

Added a test for arm-none-eabi selection
Note that the selection mechanism in the Baremetal driver is rather weak. It does not seem to respond on -mthumb, -march and -mcpu options. The cc1 options show a normalized triple, but the sub-architecture suffix of compiler_rt is the argument exactly as typed on the command line. I'm not going to fix that in this patch, but maybe we want to record this problem somewhere, so we dont forget?

A probably better approach is to use something like TC.getCompilerRT(Args, "profile"). The function returns a full path which can avoid -L problems.

std::string ToolChain::getCompilerRT(const ArgList &Args, StringRef Component,
                                     FileType Type) const {
  const llvm::Triple &TT = getTriple();
  bool IsITANMSVCWindows =
      TT.isWindowsMSVCEnvironment() || TT.isWindowsItaniumEnvironment();

  const char *Prefix =
      IsITANMSVCWindows || Type == ToolChain::FT_Object ? "" : "lib";
  const char *Suffix;
  switch (Type) {
  case ToolChain::FT_Object:
    Suffix = IsITANMSVCWindows ? ".obj" : ".o";
    break;
  case ToolChain::FT_Static:
    Suffix = IsITANMSVCWindows ? ".lib" : ".a";
    break;
  case ToolChain::FT_Shared:
    Suffix = Triple.isOSWindows()
                 ? (Triple.isWindowsGNUEnvironment() ? ".dll.a" : ".lib")
                 : ".so";
    break;
  }

  for (const auto &LibPath : getLibraryPaths()) {
    SmallString<128> P(LibPath);
    llvm::sys::path::append(P, Prefix + Twine("clang_rt.") + Component + Suffix);
    if (getVFS().exists(P))
      return std::string(P.str());
  }

  StringRef Arch = getArchNameForCompilerRTLib(*this, Args);
  const char *Env = TT.isAndroid() ? "-android" : "";
  SmallString<128> Path(getCompilerRTPath());
  llvm::sys::path::append(Path, Prefix + Twine("clang_rt.") + Component + "-" +
                                    Arch + Env + Suffix);
  return std::string(Path.str());
}

The function @MaskRay suggested does not address the problem with -march= and others, but it does sound good to use that function. I'm also not sure if -L is a feature that is relied upon at the moment by anybody that uses the baremetal driver. For the moment I'll commit this patch, but a single place across drivers that handles the compiler_rt selection mechanism certainly sounds nice.

This revision was automatically updated to reflect the committed changes.

This breaks tests on Windows: http://45.33.8.238/win/8249/step_7.txt Please take a look and if it takes a while to fix, please revert while you investigate.

I fixed windows tests in a41550cff91b7fb2b56bf0e19ccb341bfd3e37b4 . Please watch bots after landing patches next time.

I fixed windows tests in a41550cff91b7fb2b56bf0e19ccb341bfd3e37b4 . Please watch bots after landing patches next time.

Thanks for fixing my broken tests @thakis. I always relied on bots to send me email. I just realized that my github privacy settings means that there is a noreply email-address in the git commit which I assume stops buildbot from sending emails to me.