157364 2016-05-04 15:48:30 -0700 Enable Dwarf2 debug information in offline assembler for clang compiler 2016-05-09 07:56:24 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 msaboff msaboff ap commit-queue keith_miller mark.lam mitz ossy saam webkit-bug-importer oldest_to_newest 1190248 0 msaboff 2016-05-04 15:48:30 -0700 Change set r196541 added the output of Dwarf2 debugging directives to the offline assembler, but disabled the feature until the appropriate support was added to the C++ compiler. That support was added to Clang version 800.0.12 and later. We should detect that we're compiling with the appropriate clang compiler version and enable the output of the Dwarf2 directives. 1190249 1 webkit-bug-importer 2016-05-04 15:50:11 -0700 <rdar://problem/26100879> 1190270 2 278143 msaboff 2016-05-04 16:39:30 -0700 Created attachment 278143 Patch 1190272 3 278143 mark.lam 2016-05-04 16:42:11 -0700 Comment on attachment 278143 Patch rs=me 1190298 4 278143 commit-queue 2016-05-04 18:06:21 -0700 Comment on attachment 278143 Patch Clearing flags on attachment: 278143 Committed r200447: <http://trac.webkit.org/changeset/200447> 1190299 5 commit-queue 2016-05-04 18:06:25 -0700 All reviewed patches have been landed. Closing bug. 1190313 6 278143 mitz 2016-05-04 20:15:08 -0700 Comment on attachment 278143 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278143&action=review > Source/JavaScriptCore/offlineasm/config.rb:66 > + clangExecutable = ENV['TOOLCHAIN_DIR'] + '/usr/bin/clang' > + if File.executable?(clangExecutable) > + clangVersionOut = %x`#{clangExecutable} --version` The canonical way to find and invoke clang from a script running in Xcode would be simply `xcrun clang --version` 1190346 7 ossy 2016-05-05 03:54:27 -0700 (In reply to comment #4) > Comment on attachment 278143 [details] > Patch > > Clearing flags on attachment: 278143 > > Committed r200447: <http://trac.webkit.org/changeset/200447> It broke the Apple Mac Yosemite build: (see build.webkit.org for details) /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/offlineasm/config.rb:64:in `shouldEnableDebugAnnotations': undefined method `+' for nil:NilClass (NoMethodError) from /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/offlineasm/config.rb:83:in `<top (required)>' from /System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/rubygems/core_ext/kernel_require.rb:55:in `require' from /System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/rubygems/core_ext/kernel_require.rb:55:in `require' from /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:28:in `<main>' Command /bin/sh failed with exit code 1 1190347 8 278143 ossy 2016-05-05 04:03:41 -0700 Comment on attachment 278143 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278143&action=review > Source/JavaScriptCore/offlineasm/config.rb:74 > + # clang version 800.0.12 or higher is required for debug annotations > + versionMatch = /clang-(\d+).(\d+).(\d+)/.match(clangVersionOut) > + if versionMatch.length >= 4 > + totalVersion = versionMatch[1].to_i * 1000000 + versionMatch[2].to_i * 1000 + versionMatch[3].to_i > + if totalVersion >= 800000012 > + return true > + end It is not platform independent at all, it is a Mac only code. There is no GCC_VERSION, no TOOLCHAIN_DIR on non Mac platforms, no similar clang versions like this. Some Linux examples for clang --version. $ clang --version Ubuntu clang version 3.4-1ubuntu3 (tags/RELEASE_34/final) (based on LLVM 3.4) Target: x86_64-pc-linux-gnu Thread model: posix $ clang --version Ubuntu clang version 3.6.2-1 (tags/RELEASE_362/final) (based on LLVM 3.6.2) Target: x86_64-pc-linux-gnu Thread model: posix 1190357 9 ossy 2016-05-05 07:51:18 -0700 (In reply to comment #7) > (In reply to comment #4) > > Comment on attachment 278143 [details] > > Patch > > > > Clearing flags on attachment: 278143 > > > > Committed r200447: <http://trac.webkit.org/changeset/200447> > > It broke the Apple Mac Yosemite build: > (see build.webkit.org for details) > > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/offlineasm/ > config.rb:64:in `shouldEnableDebugAnnotations': undefined method `+' for > nil:NilClass (NoMethodError) > from > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/offlineasm/ > config.rb:83:in `<top (required)>' > from > /System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/ > rubygems/core_ext/kernel_require.rb:55:in `require' > from > /System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/ > rubygems/core_ext/kernel_require.rb:55:in `require' > from > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/offlineasm/ > generate_offset_extractor.rb:28:in `<main>' > Command /bin/sh failed with exit code 1 After checking the Yosemite build logs, it seems there is no TOOLCHAIN_DIR env, only DT_TOOLCHAIN_DIR. 1190360 10 mitz 2016-05-05 08:21:35 -0700 (In reply to comment #9) > (In reply to comment #7) > > (In reply to comment #4) > > > Comment on attachment 278143 [details] > > > Patch > > > > > > Clearing flags on attachment: 278143 > > > > > > Committed r200447: <http://trac.webkit.org/changeset/200447> > > > > It broke the Apple Mac Yosemite build: > > (see build.webkit.org for details) > > > > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/offlineasm/ > > config.rb:64:in `shouldEnableDebugAnnotations': undefined method `+' for > > nil:NilClass (NoMethodError) > > from > > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/offlineasm/ > > config.rb:83:in `<top (required)>' > > from > > /System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/ > > rubygems/core_ext/kernel_require.rb:55:in `require' > > from > > /System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/lib/ruby/2.0.0/ > > rubygems/core_ext/kernel_require.rb:55:in `require' > > from > > /Volumes/Data/slave/yosemite-release/build/Source/JavaScriptCore/offlineasm/ > > generate_offset_extractor.rb:28:in `<main>' > > Command /bin/sh failed with exit code 1 > > After checking the Yosemite build logs, it seems > there is no TOOLCHAIN_DIR env, only DT_TOOLCHAIN_DIR. Relying on either variable, or on the path to clang relative to it, isn’t the right thing to do. As I’ve mentioned above, xcrun is the right way to invoke clang. If this script is shared with non-Xcode build systems, then it’s probably best for the script itself to use the CC environment variable, and for the Xcode script build phase that invokes it to set that variable (if not already set) to the output of "xcrun -find clang". 1190365 11 msaboff 2016-05-05 09:35:59 -0700 Looking at the build breakage now. 1190376 12 msaboff 2016-05-05 10:15:15 -0700 Landed build fix in change set r200458: <http://trac.webkit.org/changeset/200458> 1190377 13 msaboff 2016-05-05 10:16:33 -0700 (In reply to comment #8) > Comment on attachment 278143 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=278143&action=review > > > Source/JavaScriptCore/offlineasm/config.rb:74 > > + # clang version 800.0.12 or higher is required for debug annotations > > + versionMatch = /clang-(\d+).(\d+).(\d+)/.match(clangVersionOut) > > + if versionMatch.length >= 4 > > + totalVersion = versionMatch[1].to_i * 1000000 + versionMatch[2].to_i * 1000 + versionMatch[3].to_i > > + if totalVersion >= 800000012 > > + return true > > + end > > It is not platform independent at all, it is a Mac only code. > There is no GCC_VERSION, no TOOLCHAIN_DIR on non Mac platforms, > no similar clang versions like this. > > Some Linux examples for clang --version. > > $ clang --version > Ubuntu clang version 3.4-1ubuntu3 (tags/RELEASE_34/final) (based on LLVM 3.4) > Target: x86_64-pc-linux-gnu > Thread model: posix > > $ clang --version > Ubuntu clang version 3.6.2-1 (tags/RELEASE_362/final) (based on LLVM 3.6.2) > Target: x86_64-pc-linux-gnu > Thread model: posix I intended the change to be Mac only. I have no way of determining what Linux clang versions have the multiple .file directives fix. 1190378 14 msaboff 2016-05-05 10:27:07 -0700 (In reply to comment #13) > I intended the change to be Mac only. I have no way of determining what > Linux clang versions have the multiple .file directives fix. Please add Linux clang specific check. You'll need to figure out the first clang version that has the fix given at http://reviews.llvm.org/D16101. 1191056 15 ap 2016-05-08 09:29:05 -0700 Can this be detected by a feature check, instead of a version check? It's not clear from the bug what the failure mode was. 1191178 16 msaboff 2016-05-09 07:56:24 -0700 (In reply to comment #15) > Can this be detected by a feature check, instead of a version check? > > It's not clear from the bug what the failure mode was. Without the clang fix, clang will crash with the directives this change produces. There isn't a feature check in clang for the change. 278143 2016-05-04 16:39:30 -0700 2016-05-04 18:06:21 -0700 Patch 157364.patch text/plain 2228 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjAwNDM3KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBA CisyMDE2LTA1LTA0ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIEVuYWJsZSBEd2FyZjIgZGVidWcgaW5mb3JtYXRpb24gaW4gb2ZmbGluZSBhc3NlbWJsZXIg Zm9yIGNsYW5nIGNvbXBpbGVyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xNTczNjQuCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgQWRkZWQgYSBuZXcgZnVuY3Rpb24gc2hvdWxkRW5hYmxlRGVidWdBbm5vdGF0 aW9ucygpIHRoYXQgZGV0ZXJtaW5lcyBpZgorICAgICAgICB3ZSBhcmUgdXNpbmcgY2xhbmcgYW5k IGEgbmV3IGVub3VnaCB2ZXJzaW9uIHRvIHN1cHBvcnQgdGhlIGRlYnVnIGFubm90YXRpb25zLgor CisgICAgICAgICogb2ZmbGluZWFzbS9jb25maWcucmI6CisgICAgICAgIChzaG91bGRFbmFibGVE ZWJ1Z0Fubm90YXRpb25zKTogQWRkZWQuCisKIDIwMTYtMDUtMDQgIEtlaXRoIE1pbGxlciAgPGtl aXRoX21pbGxlckBhcHBsZS5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgZml4IHRlc3QgZm9y IG5ldyBBcnJheUl0ZXJhdG9yUHJvdG90eXBlLm5leHQoKSBlcnJvciBtZXNzYWdlLgpJbmRleDog U291cmNlL0phdmFTY3JpcHRDb3JlL29mZmxpbmVhc20vY29uZmlnLnJiCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9vZmZsaW5lYXNtL2NvbmZpZy5yYgkocmV2aXNpb24gMjAw Mzk0KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL29mZmxpbmVhc20vY29uZmlnLnJiCSh3b3Jr aW5nIGNvcHkpCkBAIC01OSw0ICs1OSwyNSBAQCAkZW5hYmxlSW5zdHJBbm5vdGF0aW9ucyA9IGZh bHNlCiAjIFR1cm5zIG9uIGdlbmVyYXRpb24gb2YgRFdBUkYyIGRlYnVnIGFubm90aW9ucyBmb3Ig ZmlsZSBhbmQgbGluZSBudW1iZXJzLgogIyBBbGxvd3MgZm9yIHNvdXJjZSBsZXZlbCBkZWJ1Z2lu ZyBvZiB0aGUgb3JpZ2luYWwgLmFzbSBmaWxlcyBpbiBhIGRlYnVnZ2VyLgogIwotJGVuYWJsZURl YnVnQW5ub3RhdGlvbnMgPSBmYWxzZQorZGVmIHNob3VsZEVuYWJsZURlYnVnQW5ub3RhdGlvbnMo KQorICAgIGlmIEVOVlsnR0NDX1ZFUlNJT04nXSA9fiAvXC5jbGFuZ1wuLyBhbmQgRU5WWydUT09M Q0hBSU5fRElSJ10gIT0gJycKKyAgICAgICAgY2xhbmdFeGVjdXRhYmxlID0gRU5WWydUT09MQ0hB SU5fRElSJ10gKyAnL3Vzci9iaW4vY2xhbmcnCisgICAgICAgIGlmIEZpbGUuZXhlY3V0YWJsZT8o Y2xhbmdFeGVjdXRhYmxlKQorICAgICAgICAgICAgY2xhbmdWZXJzaW9uT3V0ID0gJXhgI3tjbGFu Z0V4ZWN1dGFibGV9IC0tdmVyc2lvbmAKKyAgICAgICAgICAgIGlmICgkPyA9PSAwKQorICAgICAg ICAgICAgICAgICMgY2xhbmcgdmVyc2lvbiA4MDAuMC4xMiBvciBoaWdoZXIgaXMgcmVxdWlyZWQg Zm9yIGRlYnVnIGFubm90YXRpb25zCisgICAgICAgICAgICAgICAgdmVyc2lvbk1hdGNoID0gL2Ns YW5nLShcZCspLihcZCspLihcZCspLy5tYXRjaChjbGFuZ1ZlcnNpb25PdXQpCisgICAgICAgICAg ICAgICAgaWYgdmVyc2lvbk1hdGNoLmxlbmd0aCA+PSA0CisgICAgICAgICAgICAgICAgICAgIHRv dGFsVmVyc2lvbiA9IHZlcnNpb25NYXRjaFsxXS50b19pICogMTAwMDAwMCArIHZlcnNpb25NYXRj aFsyXS50b19pICogMTAwMCArIHZlcnNpb25NYXRjaFszXS50b19pCisgICAgICAgICAgICAgICAg ICAgIGlmIHRvdGFsVmVyc2lvbiA+PSA4MDAwMDAwMTIKKyAgICAgICAgICAgICAgICAgICAgICAg IHJldHVybiB0cnVlCisgICAgICAgICAgICAgICAgICAgIGVuZAorICAgICAgICAgICAgICAgIGVu ZAorICAgICAgICAgICAgZW5kCisgICAgICAgIGVuZAorICAgIGVuZAorCisgICAgZmFsc2UKK2Vu ZAorCiskZW5hYmxlRGVidWdBbm5vdGF0aW9ucyA9IHNob3VsZEVuYWJsZURlYnVnQW5ub3RhdGlv bnMoKQo=