OpenBSD add C++ runtime in a driver's standpoint

[devnexen]

Summary

• Since 6.2 release, on supporters platforms clang is shipped with both libcxx and libcxxabi.

[dberris]

lib/Driver/ToolChains/CommonArgs.cpp

678–679 ↗

(On Diff #142541)

Maybe this should be in the TC.AddCXXStdlibLibArgs(...) function instead?

[devnexen]

lib/Driver/ToolChains/CommonArgs.cpp

678–679 ↗

(On Diff #142541)

I did not dare since it s the only case where it is needed (e.g. no need for X-ray for example)

[dberris]

lib/Driver/ToolChains/CommonArgs.cpp

678–679 ↗

(On Diff #142541)

Okay, let me ask this another way.

Is there ever a case for OpenBSD when we're doing TC.AddCXXStdlibLibArgs(...) that we don't need to also add these flags when OPT_pg is specified?

[devnexen]

lib/Driver/ToolChains/CommonArgs.cpp

678–679 ↗

(On Diff #142541)

I see ... will do another version then :-)

[devnexen]

• Putting the change on the driver itself.

[dberris]

Okay, can you help me understand why this isn't required for builds in OpenBSD which don't use sanitizers, but want C++ builds that also add the -pg flag? I would have thought that those situations would break, no?

[devnexen]

• Making base gcc 4.2.1 stdc++ having profile version supported as well.

[dberris]

lib/Driver/ToolChains/OpenBSD.cpp

189 ↗

(On Diff #142740)

Do you actually need this change? Why isn't getToolChain().AddCXXStdlibLibArgs(...) not sufficient here?

197–199 ↗

(On Diff #142740)

Doesn't the common args implementation already handle this case? What am I missing?

265–269 ↗

(On Diff #142740)

Almost there -- but I think you want to look at the implementation of something like the FreeBSD driver which does a suitably complete definition, which supports either libstdc++ or libc++.

That way you don't need the other change specific to when you need the sanitizer deps, and rely on the default implementation for LibFuzzer in CommonArgs. No?

Am looking at https://clang.llvm.org/doxygen/FreeBSD_8cpp_source.html#l00345 for reference.

[devnexen]

lib/Driver/ToolChains/OpenBSD.cpp

189 ↗

(On Diff #142740)

That s the thing, I wish it was simple as FreeBSD, but seemingly in OpenBSD needs both c++98 gcc runtime and libc++ for fuzzer (I tried libc++ alone already)

[dberris]

lib/Driver/ToolChains/OpenBSD.cpp

189 ↗

(On Diff #142740)

Right, but this comment is on this specific line change. I don't think you need to reach into Toolchain. direcly, since you can already use getToolChain() just from the above line (188).

[devnexen]

lib/Driver/ToolChains/OpenBSD.cpp

189 ↗

(On Diff #142740)

Right, so I guess this diff https://reviews.llvm.org/D45662?id=142686 is sufficient then ?

[dberris]

lib/Driver/ToolChains/OpenBSD.cpp

189 ↗

(On Diff #142740)

No. Let me try and explain again.

You were on the right path, with overriding the AddCXXStdlibLibArgs function in the OpenBSD Toolchain type. It's just that you weren't handling the case for when the binary was being built with libc++ or libstdc++ properly. I was referring you to what FreeBSD was doing for their implementation of AddCXXStdlibLibArgs. This means, checking first whether the invocation of the compiler was using libc++ or libstdc++, and then adding the appropriate link spelling. That all happens in the AddCXXStdlibLibArgs implementation, because there's no need to special-case just for the sanitizers.

This means, if you're building a normal binary with -pg in OpenBSD against either libc++ or libstdc++, it wouldn't work correctly regardless of whether you were using libFuzzer.

Does that make more sense?

[devnexen]

lib/Driver/ToolChains/OpenBSD.cpp

189 ↗

(On Diff #142740)

Ok will try a newer version later, thanks for your inputs.

[devnexen]

lib/Driver/ToolChains/OpenBSD.cpp

189 ↗

(On Diff #142740)

So I looked at FreeBSD and makes more sense by default it s libcxx since the 10.x releases. A release has a few years span of support. OpenBSD has complete different release policy, much shorter and two releases per year. clang been in since 6.2 (and we re at 6.3 now and the time llvm 7 comes out it will be 6.4).

[devnexen]

lib/Driver/ToolChains/OpenBSD.cpp

189 ↗

(On Diff #142740)

To complete a bit above explanation, softwares of certain version matches the OS version. In other words there won t be llvm 7 for OpenBSD 6.1

[dberris]

LGTM

Thanks for working through this @devnexen!

I can land this a little later on my day (Sydney, Australia).

cfe/trunk/lib/Driver/ToolChains/OpenBSD.h

 class LLVM_LIBRARY_VISIBILITY OpenBSD : public Generic_ELF {
 public:
   OpenBSD(const Driver &D, const llvm::Triple &Triple,
           const llvm::opt::ArgList &Args);
 
   bool IsMathErrnoDefault() const override { return false; }
   bool IsObjCNonFragileABIDefault() const override { return true; }
   bool isPIEDefault() const override { return true; }
+  void AddCXXStdlibLibArgs(const llvm::opt::ArgList &Args,
+                           llvm::opt::ArgStringList &CmdArgs) const override;
 
   unsigned GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
     return 2;
   }
   unsigned GetDefaultDwarfVersion() const override { return 2; }
 
   SanitizerMask getSupportedSanitizers() const override;
 

cfe/trunk/lib/Driver/ToolChains/OpenBSD.cpp

 
 OpenBSD::OpenBSD(const Driver &D, const llvm::Triple &Triple,
                  const ArgList &Args)
     : Generic_ELF(D, Triple, Args) {
   getFilePaths().push_back(getDriver().Dir + "/../lib");
   getFilePaths().push_back("/usr/lib");
 }
 
+void OpenBSD::AddCXXStdlibLibArgs(const ArgList &Args,
+                                  ArgStringList &CmdArgs) const {
+  bool Profiling = Args.hasArg(options::OPT_pg);
+
+  CmdArgs.push_back(Profiling ? "-lc++_p" : "-lc++");
+  CmdArgs.push_back(Profiling ? "-lc++abi_p" : "-lc++abi");
+}
+
 Tool *OpenBSD::buildAssembler() const {
   return new tools::openbsd::Assembler(*this);
 }
 
 Tool *OpenBSD::buildLinker() const { return new tools::openbsd::Linker(*this); }