[RFC] New ToolsSupport library for stuff that only tools need

There are some pieces of functionality in LLVM today, that make sense for tools like llc, opt, clang, but they aren't relevant for embedded uses like LLVM in the JavaScript JIT. One example of this type of functionality is the signal handlers.

I'd like to propose migrating content that is specifically designed for tools into a new ToolsSupport library. My initial candidates for the new library would be; anything guarded by ENABLE_CRASH_OVERRIDES, and any functionality specific to tools (i.e. ToolOutputFile and eventually command line parsing— once it can be factored out).

The primary goal of this is to make it easy for uses of LLVM that aren't command line tools to not include functionality that isn't required for that use case.

Thoughts?

-Chris

The question I have is – does the actual source code need to move? I’m wondering what aspects require a new library as opposed to different sequence of things happening when initializing LLVM as a library vs. as a tool.

(We’ve definitely had trouble with the signal handlers ourselves, but I thought we had solved it by using different code paths to set everything else up…)

There is no real technical requirement for this. Currently we work around the signal handlers by building with ENABLE_CRASH_OVERRIDES=NO, and carrying along the command line parsing code and ToolOutputFile isn’t really a big drain.

This seemed to me like the cleanest way to accomplish a goal of mine. The goal I have is to be able to build an LLVM dylib and llc/opt/etc from the same CMake invocation where the dylib does not have crash overrides and signal handler hooks, but the tools do.

There are other ways to accomplish this, but I felt that separating libLLVMSupport into two libraries seemed the cleanest. An alternate approach that could work for me would be to have the implementation of the hooks only be compiled into the tools.

-Chris

I am mostly impartial to the two proposed solutions (separate library or conditional compilation of the hooks into the tools). If we make it a library I think we should make it a convenience library that is intended to be used only with the tools and not an external supported library at all.

-Juergen

+1 to this.

I’d slightly prefer a separate library (though moving the code vs not moving the code I’m relatively apathetic about), but just because I hate conditional compilation.

-eric

Sounds like there is some interest here. I’ll work up some patches with my preferred way to handle this, and we can shake things out from there.

-Chris

I’m in favor of splitting the code out into a new directory and using separate libraries if we’re going to do this.

Build systems should be really dumb, IMO.

I was thinking of moving the files into a Tools subdirectory of Support, and having a new library generated from that. Does that mesh with what you’d like to see?

I’ve gotten wrapped into a nasty internal bug hunt, so I probably won’t have patches until next week. I will also probably need to ask Eric for help with the autoconf side of things because my familiarity with autoconf ends at “run configure”.

-Chris

So, I decided to try and start small. My idea here was create a ToolsSupport library, and move one small, but important function into the new library to shake out all the places that would need updating. The function I chose to start with was llvm::sys::PrintStackTraceOnErrorSignal.

Turns out this wasn’t so small. These patches aren’t done yet, but since they’re pretty big I thought I’d send them out for preliminary feedback about whether or not this is the right approach. The patches contain the following changes:

  • Patches to LLVM & Clang CMake build systems to add a new libLLVMToolsSupport
  • New SignalHandlers.h/.cpp/.inc files for PrintStackTraceOnErrorSignal
  • Updated all llvm tools to pull in the new library and use the new header

Still missing:

  • Windows side of the SignalHandlers patches
  • Autoconf/Make side of the build system patches
  • Patches for other LLVM projects
  • General cleanup

Feedback is greatly appreciated. I’d really like to make sure I’m not lost in the weeds on this one.

Thanks,
-Chris

libLLVMToolsSupport.diff (37.8 KB)

libLLVMToolsSupport-clang.diff (5.05 KB)

I would have preferred the library to be moved out of the “Support” folder and have its own top-level library and include folder.

Just my 2 cents.

+1, you will need to do this if you want to support the autoconf build
system.

Sorry I've been absent, but this is actually a much better summary of my
feelings here. I should have written this more clearly when I looked at the
diff, my apologies.

This should be a fairly small change to make.

That hack is the primary motivator, but that can be solved other ways. One option would be for us to move the nasty Mac-specific hack out of the cpp file and into a header that tools could optionally include.

The other thought I had which motivated this solution was that if we could strip all the functionality that is only really used by tools out into a separate library it would offer cleaner organization of code. Support seems to often get used as a dumping ground for stuff that just doesn’t fit anywhere else.

Based on your feedback and Chandler’s maybe this just isn’t the right separation. I can look into a solution to address our hackiness without creating a separate library.

-Chris

What other stuff do you think belongs in ToolsSupport that doesn't belong
in Support? Looking back at the initial email, you have command line
parsing and ToolOutputFile.

We could split out command line parsing, but it doesn't seem worth it,
given that we're still carrying regex support, Unicode conversion, dynamic
library support, and other things that probably aren't absolutely necessary.

What about splitting out a CrashRecovery library instead? That seems a lot
more targeted and meaningful. We'd probably put ToolOutputFile.cpp in there.

I think for the main goal of cleaning up the Mac-specific hack, a CrashRecovery library would work equally well. Juergen is more familiar with the WebKit side of things, so he may be aware of something I’m not thinking of.

Chandler, does splitting out a CrashRecovery library instead seem sane?

Other than code organization and naming, the general idea of splitting out a CrashRecovery library would be the same as the other patches I sent out. I was thinking of taking the approach of moving one symbol, fixing everything, then repeat.

Does that seem like the right approach?

-Chris

I think for the main goal of cleaning up the Mac-specific hack, a
CrashRecovery library would work equally well. Juergen is more familiar
with the WebKit side of things, so he may be aware of something I’m not
thinking of.

Chandler, does splitting out a CrashRecovery library instead seem sane?

I have a preference for leaving it in Support as "dead code" for the
library, and only calling routines to enable that functionality from tools.
This would of course require cleaning up any parts that are inherently bad
when linked into a library.

The reason isn't fundamental, it's just that splitting libraries,
especially as LLVM is currently set up, has a cost. Super fine-grained
libraries don't really make sense in this environment, and 'crash recovery'
seems quite fine grained and specific. Again, not bad in and of itself, but
not really a good fit in LLVM.

That's my two cents. Naturally, if it isn't *possible* to clean up the
mac-specific hacky bits and factor the crash recovery so that it is only
enabled when called, then that seems like an overriding concern. =]

I think for the main goal of cleaning up the Mac-specific hack, a CrashRecovery library would work equally well. Juergen is more familiar with the WebKit side of things, so he may be aware of something I’m not thinking of.

Chandler, does splitting out a CrashRecovery library instead seem sane?

FWIW, I prefer a ToolsSupport library. It gives us the opportunity to put other tools specific things in there if we find them. I can’t, for example, think of the dylib needing YAML right now, although I could be wrong.

At least if we made it ToolsSupport then we wouldn’t have come back in a few months/year and ask to rename it from CrashRecovery.

Pete

Here is my limited perspective on this topic:

  1. I would like to see some of the code in LLVM’s ADT and Support libraries available for reuse in other projects. This also has some potential for being useful to Clang – one could imagine a world where Clang directly linked the ADT & Support libraries, but used the compiler itself via a clear dynamic-library vended API (which would be unrealistic to expect the ADT & Support libraries would ever want to provide).

  2. I agree with Chandler in that I don’t see a good need to try hard to factor out code from the Support library that can just be conditional disabled or would be unused by normal .a link semantics. For example, whether or not the regex or YAML code belongs in Support doesn’t seem worth worrying too much about, because they are very isolated, don’t introduce extra dependencies, and won’t be linked by projects that don’t use them.

  3. I also see a good argument for having a ToolsSupport library, as a place for all of the very-LLVM(the compiler) specific functionality that would have no place in a shared Support library (for any number of reasons: lack of generality, invasiveness, or because they add substantial other dependencies).

  • Daniel
  1. I also see a good argument for having a ToolsSupport library, as a place for all of the very-LLVM(the compiler) specific functionality that would have no place in a shared Support library (for any number of reasons: lack of generality, invasiveness, or because they add substantial other dependencies).

I agree in principle, but I don’t think the signal handling or crash recovery are actually relevant for this – they’re very general, just only useful for executable applications as opposed to useful for things like the LLVM libraries in WebKit.

This assumes that the client is statically linking against Support. Chris has been pretty explicit about his goals of building and using LLVM as a monolithic dynamic library, in which case things like regex or YAML support will not be automatically removed.

—Owen