This is an archive of the discontinued LLVM Phabricator instance.

Nuke getGlobalContext() from LLVM (but the C API)
ClosedPublic

Authored by mehdi_amini on Apr 13 2016, 9:38 PM.

Details

Summary

At the same time, fixes InstructionsTest::CastInst unittest: yes
you can leave the IR in an invalid state and exit when you don't
destroy the context (like the global one), no longer now.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Nuke getGlobalContext() from LLVM (but the C API).
mehdi_amini updated this object.
mehdi_amini added reviewers: echristo, pcc.
mehdi_amini added a subscriber: llvm-commits.
mehdi_amini edited edge metadata.

Add an entry in the release notes

Red diffs are the best diffs.

lib/Target/Target.cpp
16

This tends to be a compile time disaster. Can you add a comment that it is for the global context and that we want to kill it eventually ?

Maybe you could declare the function locally instead of importing the whole Core.h thing ?

Fwd-declare instead of including "llvm-c/Core.h"

Add missing extern "C" in fwd-decl.

This looks awesome, but maybe split this in two? One changes all uses and
another does the removal just in case we find something in another project?

echristo accepted this revision.Apr 14 2016, 1:29 PM
echristo edited edge metadata.

... This patch is hilarious.

OK.

-eric

This revision is now accepted and ready to land.Apr 14 2016, 1:29 PM
mehdi_amini closed this revision.Apr 14 2016, 4:35 PM

r266379 for updating all the uses.
r266380 for nuking the API.

chapuni added inline comments.
tools/lli/lli.cpp
388

I guess it might cause undefined behavior. See do_shutdown().
It seems EE depends on LLVMContext and EE is alive after Context has been destroyed.

Appeased in r266441, or we should do;

  1. Make destructors in EE to be unaware of LLVMContext.
  2. Don't use return in main(), but exit().
  3. Make lli not to use do_shutdown().

Tried to clean it in r266652 by removing global variables in lli.

Thanks for the notification!

If anyone comes here looking for a fix on the client side, there is a very easy solution:

LLVMContext &getMyLLVMGlobalContext() {
  static LLVMContext MyGlobalContext;
  return MyGlobalContext;
}

I encourage people to avoid global mutable state in general though :)