Update the Import Script to Include Windows Headers #14

gmittert · 2019-03-21T21:48:31Z

Update the import script and run it to import a new version of llvm. The only file changes are in the first commit. Everything in the second commit should be identical to swift llvm commit ab94816b593daf1c623322b0fb981abb0b156833 except for llvm/Support/Process.cpp with required some hacks in the import-llvm script.

gmittert · 2019-03-21T21:51:52Z

Indexstore includes llvm/ADT/OptionSet.h, which from as far as I can tell is from this patch that never got merged. I've just copied it out and have the import script copy it back in. However really, either the uses of it should be removed, or it should be necroed an merged it.

benlangmuir · 2019-03-25T18:52:33Z

Instead of moving the import script, I suggest using a different name for the directory containing the files. That will also make it a lot easier to review the changes to that script.

gmittert · 2019-03-25T20:18:20Z

Alright, modified this one to just be the import script changes. I should be much easier to read now.

benlangmuir · 2019-03-25T18:48:58Z

lib/LLVMSupport/Support/Process.cpp

@@ -1,3 +1,4 @@
+#define LLVM_ENABLE_CRASH_DUMPS false


Is this the change that was required? It should go into config.h.

benlangmuir · 2019-03-25T20:32:10Z

Utilities/import-llvm


 # ADT "Small" structures.
 ADT_imports += ['SmallPtrSet', 'SmallSet', 'SmallString', 'SmallVector']

 # ADT Algorithms.
 ADT_imports += ['edit_distance']

+Demangle_imports = ['Demangle', 'ItaniumDemangle', 'MicrosoftDemangle',


What pulled in the demangler?

Support/Unix/Signals.inc includes Demangle.h it uses it to demangle symbols when printing backtraces.

Ah, this was apparently one of the modifications to the files that we warned about at the top of the import script:

char* d = nullptr;//itaniumDemangle(dlinfo.dli_sname, nullptr, nullptr, &res);

I don't think we want to pull in the demangler.

benlangmuir · 2019-03-25T20:35:47Z

Utilities/import-llvm

+    os.symlink(os.path.join('..', '..', '..', 'include', 'llvm'),
+               os.path.join(sourcekit_llvm_lib, 'include', 'llvm'))
+    os.symlink(os.path.join('..', '..', '..', 'include', 'llvm-c'),
+               os.path.join(sourcekit_llvm_lib, 'include', 'llvm-c'))


Could these symlinks be done by looping over the directories instead of listing them out manually? It's really hard to tell if these are correct by inspection.

Cleaned these up a bit, I didn't need as many as I thought I did.

benlangmuir · 2019-03-25T20:39:46Z

Utilities/import-llvm

+    copyfile(os.path.join(sourcekit_srcroot, 'include', 'llvm', 'Support', 'AArch64TargetParser.def'),
+             os.path.join(def_location, 'AArch64TargetParser.def'))
+
+    # Process.cpp uses LLVM_ENABLE_CRASH_DUMPS which is usually set in CMake.


This should be in config.h. In general, it would be good to look at a new config.h from an llvm build, although it will need to be merged manually with what's here, which is already customized. But that more general change doesn't need to happen in this PR.

benlangmuir · 2019-03-25T20:41:56Z

Utilities/import-llvm

+    if os.path.exists(def_location):
+        shutil.rmtree(def_location)
+    os.makedirs(def_location)
+    copyfile(os.path.join(sourcekit_srcroot, 'include', 'llvm', 'Support', 'AArch64TargetParser.def'),


Does the original location get looked at? If not, we could move instead of copy.

Move it is!

benlangmuir · 2019-03-25T20:46:11Z

Utilities/import-llvm

-               'Statistic', 'StringExtras', 'STLExtras', 'AllocatorList', 'Triple']
+ADT_imports = ['iterator', 'iterator_range', 'APFloat', 'APInt', 'APSInt',
+               'AllocatorList', 'EpochTracker', 'Hashing', 'None', 'Optional',
+               'Statistic', 'StringExtras', 'STLExtras']


These lists are getting quite messy. I think we're better off just making each variable into a single list, and putting every dependency on its own line. That will make it much easier to see changes in the future. Splitting it up "thematically" like this made more sense when we had fewer imports and you could still conceivable eyeball changes.

benlangmuir · 2019-03-25T23:15:36Z

The TimeValue changes seem to be here too.

gmittert · 2019-03-25T23:16:25Z

Utilities/import-llvm

-    'FormatVariadicDetails', 'NativeFormatting', 'DJB', 'ReverseIteration', 'MD5',
-    'SmallVectorMemoryBuffer', 'WithColor', 'Options', 'PrettyStackTrace', 'Watchdog',
-    'TargetParser', 'ARMBuildAttributes', 'ARMTargetParser.def', 'AArch64TargetParser.def', 'X86TargetParser.def', 'LineIterator']
+    'AArch64TargetParser',


Praise be M-x sort-lines

gmittert · 2019-03-25T23:19:21Z

The TimeValue changes seem to be here too.

Had them in the branch to make sure things compiled, rebased off master, gone now.

benlangmuir · 2019-03-25T23:49:22Z

Utilities/import-llvm

+    'Process',
+    'Program',
+    'Recycler',
+    'Regex',


What is pulling in Regex (and all the reg* files below)? If possible it would be nice to avoid this.

Timer -> YAML (so it can print JSON) -> Regex. It's just one function that indexstoredb doesn't use, so it could be commented out/removed.

gmittert · 2019-03-26T22:04:53Z

Alright, added in some super gross sed calls so that Demangle and Regex don't need to be imported.

benlangmuir · 2019-04-01T18:25:07Z

Utilities/import-llvm

+
+    # Timer.cpp and Statistics.cpp use YAMLTraits to assert some JSON is valid. We don't want to
+    # pull in the YAML/Regex so we'll remove the "offending" lines.
+    timer_location = os.path.join(sourcekit_srcroot, 'lib', 'LLVMSupport', 'Support', 'Timer.cpp')


Yikes, this is pretty gross. Could we use a patch file instead of doing this with sed?

Sure, that seems like a much nice way to do it.

Additionally, update it so that it's compatibile with the current stable branch (ab94816b593daf1c623322b0fb981abb0b156833)

gmittert · 2019-04-15T18:14:50Z

Ping on this one

akyrtzi · 2019-04-30T06:07:17Z

Apologies, this PR slipped my mind, and I've already gone ahead and imported a new llvm here: #18

I introduced a patch file to avoid adding the regex dependency and to fix include errors (ideally the patch file changes should go to llvm upstream).

I avoided adding the demangle dependency by adding a dummy Demangle.h

I also added the Windows support files.
If there is something missing for Windows support please open a new PR against master.

gmittert requested review from akyrtzi and benlangmuir as code owners March 21, 2019 21:48

gmittert changed the title ~~Run script~~ Update to the latest LLVM and Include Windows Headers Mar 21, 2019

gmittert force-pushed the RunScript branch from 0ea4261 to a7a01d8 Compare March 25, 2019 20:16

gmittert changed the title ~~Update to the latest LLVM and Include Windows Headers~~ Update the Import Script to Include Windows Headers Mar 25, 2019

gmittert mentioned this pull request Mar 25, 2019

Run the llvm-import Script to Update to a new LLVM #15

Closed

benlangmuir reviewed Mar 25, 2019

View reviewed changes

gmittert force-pushed the RunScript branch 2 times, most recently from 161f73d to f2f1c1b Compare March 25, 2019 23:14

gmittert commented Mar 25, 2019

View reviewed changes

gmittert force-pushed the RunScript branch from f2f1c1b to 9f30bd0 Compare March 25, 2019 23:18

benlangmuir reviewed Mar 25, 2019

View reviewed changes

gmittert force-pushed the RunScript branch 2 times, most recently from 558d1e0 to c54f669 Compare March 26, 2019 22:03

gmittert force-pushed the RunScript branch from c54f669 to 454635a Compare March 26, 2019 22:13

benlangmuir reviewed Apr 1, 2019

View reviewed changes

Update the LLVM Import Script to include Windows Support

c1adf5b

Additionally, update it so that it's compatibile with the current stable branch (ab94816b593daf1c623322b0fb981abb0b156833)

gmittert force-pushed the RunScript branch from 454635a to c1adf5b Compare April 1, 2019 19:38

akyrtzi closed this Apr 30, 2019

Update the Import Script to Include Windows Headers #14

Update the Import Script to Include Windows Headers #14

Uh oh!

Conversation

gmittert commented Mar 21, 2019

Uh oh!

gmittert commented Mar 21, 2019

Uh oh!

benlangmuir commented Mar 25, 2019

Uh oh!

gmittert commented Mar 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benlangmuir commented Mar 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmittert commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmittert commented Mar 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmittert commented Apr 15, 2019

Uh oh!

akyrtzi commented Apr 30, 2019

Uh oh!

Uh oh!

gmittert commented Mar 25, 2019 •

edited

Loading