assigned to @MatzeB

I think I already got an internal report about a similar issue. And I believe it requires https://reviews.llvm.org/D27559 to be fixed. I'll revert for now as there don't seem to be any reviews coming in on that other patch.

Reverted in r289570 for now.

Someone over here tried https://reviews.llvm.org/D27559 on top of your change and apparently it didn't help.

Running stuff on iOS apparently requires certs and whatnot and I can't do it, but one failing test was this EXPECT_EQ:
https://cs.chromium.org/chromium/src/base/strings/string_number_conversions_unittest.cc?q=StringNumberConversionsTest&sq=package:chromium&dr=C&l=711

So you can probably repro by having two files a.cc b.cc looking roughly like so:

$ cat a.cc
#include
#include
#include <stdint.h>
bool HexStringToBytes(const std::string& input, std::vector<uint8_t>* output);
int main() {
static const struct {
const std::string input;
const char* output;
size_t output_len;
bool success;
} cases[] = {
{"0", "", 0, false}, // odd number of characters fails
{"00", "\0", 1, true},
{"42", "\x42", 1, true},
{"-42", "", 0, false}, // any non-hex value fails
{"+42", "", 0, false},
{"7fffffff", "\x7f\xff\xff\xff", 4, true},
{"80000000", "\x80\0\0\0", 4, true},
{"deadbeef", "\xde\xad\xbe\xef", 4, true},
{"DeadBeef", "\xde\xad\xbe\xef", 4, true},
{"0x42", "", 0, false}, // leading 0x fails (x is not hex)
{"0f", "\xf", 1, true},
{"45 ", "\x45", 1, false},
{"efgh", "\xef", 1, false},
{"", "", 0, false},
{"0123456789ABCDEF", "\x01\x23\x45\x67\x89\xAB\xCD\xEF", 8, true},
{"0123456789ABCDEF012345",
"\x01\x23\x45\x67\x89\xAB\xCD\xEF\x01\x23\x45", 11, true},
};

for (size_t i = 0; i < arraysize(cases); ++i) {
std::vector<uint8_t> output;
std::vector<uint8_t> compare;
HexStringToBytes(cases[i].input, &output;
for (size_t j = 0; j < cases[i].output_len; ++j)
compare.push_back(static_cast<uint8_t>(cases[i].output[j]));
//ASSERT_EQ(output.size(), compare.size()) << i << ": " << cases[i].input;
if (!std::equal(output.begin(), output.end(), compare.begin()))
return 1; // if this happens, things are bad
}
}
$ cat b.cc
#include
#include
#include <stdint.h>
// Utility to convert a character to a digit in a given base
template<typename CHAR, int BASE, bool BASE_LTE_10> class BaseCharToDigit {
};

// Faster specialization for bases <= 10
template<typename CHAR, int BASE> class BaseCharToDigit<CHAR, BASE, true> {
public:
static bool Convert(CHAR c, uint8_t* digit) {
if (c >= '0' && c < '0' + BASE) {
*digit = static_cast<uint8_t>(c - '0');
return true;
}
return false;
}
};

// Specialization for bases where 10 < base <= 36
template<typename CHAR, int BASE> class BaseCharToDigit<CHAR, BASE, false> {
public:
static bool Convert(CHAR c, uint8_t* digit) {
if (c >= '0' && c <= '9') {
*digit = c - '0';
} else if (c >= 'a' && c < 'a' + BASE - 10) {
*digit = c - 'a' + 10;
} else if (c >= 'A' && c < 'A' + BASE - 10) {
*digit = c - 'A' + 10;
} else {
return false;
}
return true;
}
};

template <int BASE, typename CHAR>
bool CharToDigit(CHAR c, uint8_t* digit) {
return BaseCharToDigit<CHAR, BASE, BASE <= 10>::Convert(c, digit);
}
template
bool HexStringToBytesT(const STR& input, std::vector<uint8_t>* output) {
//DCHECK_EQ(output->size(), 0u);
size_t count = input.size();
if (count == 0 || (count % 2) != 0)
return false;
for (uintptr_t i = 0; i < count / 2; ++i) {
uint8_t msb = 0; // most significant 4 bits
uint8_t lsb = 0; // least significant 4 bits
if (!CharToDigit<16>(input[i * 2], &msb) ||
!CharToDigit<16>(input[i * 2 + 1], &lsb))
return false;
output->push_back((msb << 4) | lsb);
}
return true;
}
bool HexStringToBytes(const std::string& input, std::vector<uint8_t>* output) {
return HexStringToBytesT(input, output);
}

As far as I can tell, we build with -O2 and some other flags (partial list -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -fcolor-diagnostics -arch arm64 -miphoneos-version-min=9.0 -fvisibility=hidden -fno-threadsafe-statics -fvisibility-inlines-hidden -std=c++11 -fno-rtti -fno-exceptions). Maybe that's enough for you to repro?

I could not reproduce the problem with this. After fixing some smallish syntax errors I get the same results with/without the patches except for a different order of the .loh statements which has no effect.

Thinking about what could go wrong, looks like https://reviews.llvm.org/D27558 is probably also required to fix potentially wrong live-in blocks. Anyway I suggest waiting until the reviews on those patches are done and I landed the series in tree and then test again.

We now have a bot that builds chrome/iOS with clang trunk continously. It was green but then ~all tests started failing again. The last good run was at r290024, the first bad with r290033. Your reland is r290026, so it looks like it still breaks things. So you probably want to revert again :-(

I apologize for not getting you a good repro. I'll try to obtain an iOS device on Monday and get you one myself.

Sorry for breaking you again, I wish one of LNT or test-suite bots would catch the bug but apparently not.

In the mean time reverted in r290047.

Current status:

• managed to obtain iOS device
• even managed to activate it despite lack of SIM card (by temporarily finding a donor sim card)
• managed to build chrome's base_unittests for iOS
• managed to get code signing mostly figured out
• so far haven't managed to set up provisioning for iOS device

(I'm actively looking at this but the iOS part is putting up more fight than expected, sorry)

Hooray, I managed to run the test on an iPhone device and I managed to reproduce the test failure. I'm reducing now.

If anyone wants to follow along at home, follow https://chromium.googlesource.com/chromium/src/+/master/docs/ios_build_instructions.md up to "Setting up the build".

Then edit

out/Release-iphoneos/args.gn to look like:

is_debug = false
target_cpu = "arm64"
target_os = "ios"
ios_app_bundle_id_prefix = "com.google"
ios_automatically_manage_certs = false

clang_use_chrome_plugins = false
clang_base_path = getenv("HOME") + "/src/llvm-build2"

Then ninja -C out/Release-iphoneos/ base_unittests (you'll have to change the prefix and have certs and whatnot), and then open out/build/all.xcworkspace, pick base_unittests on the left, the device at the top, and hit play. Test output is in Xcode's console.

It's proving somewhat difficult to reduce. Here's what I have so far (I'll keep trying), maybe that's already enough to give you an idea. I'm down to 15 seemingly unrelated files in base_unittests (plus still all of its dependencies) -- https://codereview.chromium.org/2595063002/ if you're really curious. It feels like a certain size is needed so that string literal addressing is done via adrp/add instead of just ldr'ing off an immediate address. (I'll try to get the same effect by putting in generated filler stuff.)

Take a look at the RHS of https://codereview.chromium.org/2595063002/diff/1/base/strings/string_number_conversions_unittest.cc , the failing test (also shown in comment 3).

The first thing it does is load a few string literals into std::strings on the stack (input in comment 3).

In a working binary, with your change reverted, this looks for example like so:

0000000100006d10 adrp x9, 256 ; 0x100006000
0000000100006d14 add x9, x9, #​3688 ; literal pool for: "0123456789ABCDEF"
0000000100006d18 ldr q0, [x9]
0000000100006d1c str q0, [x8, #​672]
0000000100006d20 strb wzr, [x8, #​688]
0000000100006d24 adrp x9, 256 ; 0x100006000
0000000100006d28 add x9, x9, #​3705 ; literal pool for: "#Eg�«Íï"
0000000100006d2c str x9, [x8, #​696]
0000000100006d30 str x11, [x8, #​704]
0000000100006d34 strb w20, [x8, #​712]
0000000100006d38 movz w9, #​0x16
0000000100006d3c strb w9, [x8, #​743]
0000000100006d40 adrp x9, 256 ; 0x100006000
0000000100006d44 add x9, x9, #​3714 ; literal pool for: "0123456789ABCDEF012345"
0000000100006d48 add x10, x8, #​734
0000000100006d4c ldur x11, [x9, #​14]
0000000100006d50 str x11, [x10]
0000000100006d54 ldr q0, [x9]
0000000100006d58 str q0, [x8, #​720]
0000000100006d5c strb wzr, [x8, #​742]

and so on. Strings are loaded pc-relative, and the stored to the stack in very few instructions thanks to the wide amd64 registers.

Here's the same snippet with your patch applied:

0000000100006d9c adrp x9, 256 ; 0x100006000
0000000100006da0 add x9, x9, #​3704 ; literal pool for: "0123456789ABCDEF"
0000000100006da4 ldr q0, [x9]
0000000100006da8 str q0, [x8, #​672]
0000000100006dac strb wzr, [x8, #​688]
0000000100006db0 nop
0000000100006db4 add x9, x9, #​3721
0000000100006db8 str x9, [x8, #​696]
0000000100006dbc str x11, [x8, #​704]
0000000100006dc0 strb w20, [x8, #​712]
0000000100006dc4 movz w9, #​0x16
0000000100006dc8 strb w9, [x8, #​743]
0000000100006dcc adrp x9, 256 ; 0x100006000
0000000100006dd0 add x9, x9, #​3730 ; literal pool for: "0123456789ABCDEF012345"
0000000100006dd4 add x10, x8, #​734
0000000100006dd8 ldur x11, [x9, #​14]
0000000100006ddc str x11, [x10]
0000000100006de0 ldr q0, [x9]
0000000100006de4 str q0, [x8, #​720]
0000000100006de8 strb wzr, [x8, #​742]

This looks really similar, except that the pc-rel load of the "output" array ("\x01\x23\x45\x67\x89\xAB\xCD\xEF" in the source) changes from

0000000100006d24 adrp x9, 256 ; 0x100006000
0000000100006d28 add x9, x9, #​3705 ; literal pool for: "#Eg�«Íï"

to

0000000100006db0 nop
0000000100006db4 add x9, x9, #​3721

-- the adrp got lost! I think that's the root cause, but I haven't reduced things yet.

If I use clang with your patch applied and try to reduce the number of code in the repro case more, then the above becomes something like

0000000100007218 ldr q0, 0x100106e98
000000010000721c str q0, [x8, #​672]
0000000100007220 strb wzr, [x8, #​688]
0000000100007224 adr x9, #​1047685 ; literal pool for: "#Eg�«Íï"
0000000100007228 nop
000000010000722c str x9, [x8, #​696]
0000000100007230 str x11, [x8, #​704]
0000000100007234 strb w20, [x8, #​712]
0000000100007238 movz w9, #​0x16
000000010000723c strb w9, [x8, #​743]
0000000100007240 adr x9, #​1047666 ; literal pool for: "0123456789ABCDEF012345"
0000000100007244 nop
0000000100007248 add x10, x8, #​734
000000010000724c ldur x11, [x9, #​14]
0000000100007250 str x11, [x10]
0000000100007254 ldr q0, [x9]
0000000100007258 str q0, [x8, #​720]
000000010000725c strb wzr, [x8, #​742]

i.e. this now uses either ldr to load an immediate address, or uses adr instead of adrp because the string literal is close enough to pc that that can work, and that hides the problem.

Does this ring any bells?

This seems to be on the right track. If I add another function that expands to > 1MB of code then I can remove all other source files from base_unittests and still repro. Here's my one file in base_unittests (it still links to a bunch of static libraries for now, but I'm relatively confident I can reduce unhindered from now):

#include "base/strings/string_number_conversions.h"

#include

#include "testing/gtest/include/gtest/gtest.h"

namespace base {

TEST(StringNumberConversionsTest, HexStringToBytes) {
static const struct {
const std::string input;
const char* output;
size_t output_len;
bool success;
} cases[] = {
{"0", "", 0, false}, // odd number of characters fails
{"00", "\0", 1, true},
{"42", "\x42", 1, true},
{"-42", "", 0, false}, // any non-hex value fails
{"+42", "", 0, false},
{"7fffffff", "\x7f\xff\xff\xff", 4, true},
{"80000000", "\x80\0\0\0", 4, true},
{"deadbeef", "\xde\xad\xbe\xef", 4, true},
{"DeadBeef", "\xde\xad\xbe\xef", 4, true},
{"0x42", "", 0, false}, // leading 0x fails (x is not hex)
{"0f", "\xf", 1, true},
{"45 ", "\x45", 1, false},
{"efgh", "\xef", 1, false},
{"", "", 0, false},
{"0123456789ABCDEF", "\x01\x23\x45\x67\x89\xAB\xCD\xEF", 8, true},
{"0123456789ABCDEF012345",
"\x01\x23\x45\x67\x89\xAB\xCD\xEF\x01\x23\x45", 11, true},
};

for (size_t i = 0; i < arraysize(cases); ++i) {
std::vector<uint8_t> output;
std::vector<uint8_t> compare;
EXPECT_EQ(cases[i].success, HexStringToBytes(cases[i].input, &output)) <<
i << ": " << cases[i].input;
for (size_t j = 0; j < cases[i].output_len; ++j)
compare.push_back(static_cast<uint8_t>(cases[i].output[j]));
ASSERT_EQ(output.size(), compare.size()) << i << ": " << cases[i].input;
EXPECT_TRUE(std::equal(output.begin(), output.end(), compare.begin())) <<
i << ": " << cases[i].input;
}
}

TEST(StringNumberConversionsTestPadding, FooBar) {
// Make sure we have 1MB of code, so that the string accesses above have to
// use adrp instead of adr.
volatile int i = 0;
if (i) { asm(".zero 1048576"); }
}

} // namespace base

(thanks to pcc who mentioned that code stuffing trick on irc)

A'ight, this two-file program prints "shame" and exit(1)s with your patch and just runs fine without it. I'm also pasting the full build log, some of which is our goop for setting up a dummy iOS bundle to run regular executables on iOS. You probably have something similar. The only thing really interesting for you is compiler and linker flags, I'm guessing.

I think this should be enough for you to repro, let me know if you need more.

thakis-macpro:src thakis$ cat base/strings/string_number_conversions.cc
#include
#include
#include <stdint.h>

bool CharToDigit(char c, uint8_t* digit) {
if (c >= '0' && c <= '9') {
*digit = c - '0';
} else if (c >= 'a' && c < 'a' + 16 - 10) {
*digit = c - 'a' + 10;
} else if (c >= 'A' && c < 'A' + 16 - 10) {
*digit = c - 'A' + 10;
} else {
return false;
}
return true;
}

bool HexStringToBytes(const std::string& input, std::vector<uint8_t>* output) {
size_t count = input.size();
if (count == 0 || (count % 2) != 0)
return false;
for (uintptr_t i = 0; i < count / 2; ++i) {
uint8_t msb = 0; // most significant 4 bits
uint8_t lsb = 0; // least significant 4 bits
if (!CharToDigit(input[i * 2], &msb) ||
!CharToDigit(input[i * 2 + 1], &lsb))
return false;
output->push_back((msb << 4) | lsb);
}
return true;
}

void f() {
// Make sure we have 1MB of code, so that the string accesses above have to
// use adrp instead of adr.
asm(".zero 1048576");
}

thakis-macpro:src thakis$ cat base/strings/string_number_conversions_unittest.cc
#include
#include

bool HexStringToBytes(const std::string& input, std::vector<uint8_t>* output);
void f();

int main() {
static const struct {
const std::string input;
const char* output;
size_t output_len;
bool success;
} cases[] = {
{"0", "", 0, false}, // odd number of characters fails
{"00", "\0", 1, true},
{"42", "\x42", 1, true},
{"-42", "", 0, false}, // any non-hex value fails
{"+42", "", 0, false},
{"7fffffff", "\x7f\xff\xff\xff", 4, true},
{"80000000", "\x80\0\0\0", 4, true},
{"deadbeef", "\xde\xad\xbe\xef", 4, true},
{"DeadBeef", "\xde\xad\xbe\xef", 4, true},
{"0x42", "", 0, false}, // leading 0x fails (x is not hex)
{"0f", "\xf", 1, true},
{"45 ", "\x45", 1, false},
{"efgh", "\xef", 1, false},
{"", "", 0, false},
{"0123456789ABCDEF", "\x01\x23\x45\x67\x89\xAB\xCD\xEF", 8, true},
{"0123456789ABCDEF012345",
"\x01\x23\x45\x67\x89\xAB\xCD\xEF\x01\x23\x45", 11, true},
};

for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); ++i) {
std::vector<uint8_t> output;
std::vector<uint8_t> compare;
if (cases[i].success != HexStringToBytes(cases[i].input, &output)) {
fprintf(stderr, "shame\n");
exit(1);
}

for (size_t j = 0; j < cases[i].output_len; ++j)
  compare.push_back(static_cast<uint8_t>(cases[i].output[j]));

if (output.size() != compare.size()) {
  fprintf(stderr, "shame\n");
  exit(1);
}

if (!std::equal(output.begin(), output.end(), compare.begin())) {
  fprintf(stderr, "shame\n");
  exit(1);
}

}

volatile int i = 0; if (i) f(); // Don't dead-strip f.
}

thakis-macpro:src thakis$ time ninja -C out/Release-iphoneos/ base_unittests -v
ninja: Entering directory `out/Release-iphoneos/'
[1/23] touch obj/build/config/sanitizers/deps_no_options.stamp
[2/23] touch obj/base/base_unittests_generate_info_plist_merge.inputdeps.stamp
[3/23] touch obj/build/config/sanitizers/deps.stamp
[4/23] rm -rf base_unittests.app/Default.png && if [[ -d ../../testing/gtest_ios/Default.png ]]; then mkdir -p base_unittests.app/Default.png && cd ../../testing/gtest_ios/Default.png && pax -rwl . "$OLDPWD"/base_unittests.app/Default.png; else ln -f ../../testing/gtest_ios/Default.png base_unittests.app/Default.png 2>/dev/null || (rm -rf base_unittests.app/Default.png && cp -af ../../testing/gtest_ios/Default.png base_unittests.app/Default.png); fi
[5/23] python ../../build/config/mac/plist_util.py merge -f=binary1 -o=gen/base/base_unittests_generate_info_plist_merged.plist ../../build/config/ios/BuildInfo.plist ../../testing/gtest_ios/unittest-Info.plist
[6/23] touch obj/base/base_unittests_generate_info_plist_merge.stamp
[7/23] touch obj/base/base_unittests_generate_info_plist.inputdeps.stamp
[8/23] python ../../build/config/mac/plist_util.py substitute -f=binary1 -o=gen/base/base_unittests_generate_info_plist.plist -t=gen/base/base_unittests_generate_info_plist_merged.plist -s=BUILD_MACHINE_OS_BUILD=15G1212 -s=EXECUTABLE_NAME=base_unittests -s=GCC_VERSION=com.apple.compilers.llvm.clang.1_0 -s=PRODUCT_NAME=base_unittests -s=XCODE_BUILD=8B62 -s=XCODE_VERSION=0810 -s=GTEST_BUNDLE_ID_SUFFIX=base_unittests -s=IOS_BUNDLE_ID_PREFIX=com.google -s=IOS_DEPLOYMENT_TARGET=9.0 -s=IOS_PLATFORM_BUILD=14B72 -s=IOS_PLATFORM_NAME=iphoneos -s=IOS_PLATFORM_VERSION=10.1 -s=IOS_SDK_BUILD=14B72 -s=IOS_SDK_NAME=iphoneos10.1 -s=IOS_SUPPORTED_PLATFORM=iPhoneOS
[9/23] touch obj/base/base_unittests_generate_info_plist.stamp
[10/23] rm -rf base_unittests.app/Info.plist && if [[ -d gen/base/base_unittests_generate_info_plist.plist ]]; then mkdir -p base_unittests.app/Info.plist && cd gen/base/base_unittests_generate_info_plist.plist && pax -rwl . "$OLDPWD"/base_unittests.app/Info.plist; else ln -f gen/base/base_unittests_generate_info_plist.plist base_unittests.app/Info.plist 2>/dev/null || (rm -rf base_unittests.app/Info.plist && cp -af gen/base/base_unittests_generate_info_plist.plist base_unittests.app/Info.plist); fi
[11/23] touch obj/base/base_unittests_pkg_info.inputdeps.stamp
[12/23] python ../../build/config/mac/write_pkg_info.py --plist gen/base/base_unittests_generate_info_plist.plist --output gen/base/base_unittests_pkg_info
[13/23] rm -rf base_unittests.app/PkgInfo && if [[ -d gen/base/base_unittests_pkg_info ]]; then mkdir -p base_unittests.app/PkgInfo && cd gen/base/base_unittests_pkg_info && pax -rwl . "$OLDPWD"/base_unittests.app/PkgInfo; else ln -f gen/base/base_unittests_pkg_info base_unittests.app/PkgInfo 2>/dev/null || (rm -rf base_unittests.app/PkgInfo && cp -af gen/base/base_unittests_pkg_info base_unittests.app/PkgInfo); fi
[14/23] ../../../../llvm-build2/bin/clang++ -MMD -MF obj/base/base_unittests_arch_executable_sources/string_number_conversions.o.d -DV8_DEPRECATION_WARNINGS -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=289944-1 -DCR_XCODE_VERSION=0810 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../.. -Igen -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -fcolor-diagnostics -arch arm64 -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Os -gdwarf-2 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS10.1.sdk -stdlib=libc++ -miphoneos-version-min=9.0 -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -fno-threadsafe-statics -fvisibility-inlines-hidden -std=c++11 -fno-rtti -fno-exceptions -c ../../base/strings/string_number_conversions.cc -o obj/base/base_unittests_arch_executable_sources/string_number_conversions.o
[15/23] ../../../../llvm-build2/bin/clang++ -MMD -MF obj/base/base_unittests_arch_executable_sources/string_number_conversions_unittest.o.d -DV8_DEPRECATION_WARNINGS -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=289944-1 -DCR_XCODE_VERSION=0810 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../.. -Igen -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -fcolor-diagnostics -arch arm64 -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Os -gdwarf-2 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS10.1.sdk -stdlib=libc++ -miphoneos-version-min=9.0 -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -fno-threadsafe-statics -fvisibility-inlines-hidden -std=c++11 -fno-rtti -fno-exceptions -c ../../base/strings/string_number_conversions_unittest.cc -o obj/base/base_unittests_arch_executable_sources/string_number_conversions_unittest.o
[16/23] touch obj/base/base_unittests_arch_executable_sources.stamp
[17/23] TOOL_VERSION=1479159887 ../../build/toolchain/mac/linker_driver.py ../../../../llvm-build2/bin/clang++ -Xlinker -rpath -Xlinker @​executable_path/Frameworks -Xlinker -objc_abi_version -Xlinker 2 -arch arm64 -Werror -Wl,-dead_strip -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS10.1.sdk -stdlib=libc++ -miphoneos-version-min=9.0 -Wl,-ObjC -o "obj/base/arm64/base_unittests" -Wl,-filelist,"obj/base/arm64/base_unittests.rsp" -framework UIKit -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation
[18/23] touch obj/base/base_unittests_executable.inputdeps.stamp
[19/23] python ../../build/toolchain/mac/linker_driver.py xcrun lipo -create -output obj/base/base_unittests obj/base/arm64/base_unittests
[20/23] touch obj/base/base_unittests_executable.stamp
[21/23] touch obj/base/base_unittests.codesigning.inputdeps.stamp
[22/23] python ../../build/config/ios/codesign.py code-sign-bundle -t=iphoneos -i=E4F29B16FFF03137BD658E870CABC14FD4961B7F -e=../../build/config/ios/entitlements.plist -b=obj/base/base_unittests base_unittests.app
[23/23] touch obj/base/base_unittests.stamp

Oh, and comment 9 explains why this is crashing. Why that adrp is getting lost isn't clear though :-)

It looks like it's still there in the .o file. I also can't reproduce this in an Android arm64 build. Maybe this is running into some ld64 relocation processing bug?

Thanks for reducing this, I'll dive into it in the next days.

For context: The whole LOH pass computes extra information for the linker that allows it to remove some instructions that become unnecessary after resolving relocations. AArch64CollectLOH.cpp has explanations at the beginning if you are interested in more details. This optimization is only available for mach-o so android is not affected.

I am still unable to reproduce this in my setup :-(

Could you do the following for me:

1. Copy the string_number_conversions.cc and string_number_conversions_unittest.cc (the reduced ones here) into a directory
2. Run the following two commands:

$ export SYSROOT="$(xcrun --sdk iphoneos --show-sdk-path)"
$ clang++ -DV8_DEPRECATION_WARNINGS -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=289944-1 -DCR_XCODE_VERSION=0810 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../.. -Igen -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -fcolor-diagnostics -arch arm64 -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Os -gdwarf-2 -isysroot ${SYSROOT} -stdlib=libc++ -miphoneos-version-min=9.0 -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -fno-threadsafe-statics -fvisibility-inlines-hidden -std=c++11 -fno-rtti -fno-exceptions -emit-llvm -S string_number_conversions.cc -o string_number_conversions.ll

$ clang++ -DV8_DEPRECATION_WARNINGS -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=289944-1 -DCR_XCODE_VERSION=0810 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../.. -Igen -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -fcolor-diagnostics -arch arm64 -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Os -gdwarf-2 -isysroot ${SYSROOT} -stdlib=libc++ -miphoneos-version-min=9.0 -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -fno-threadsafe-statics -fvisibility-inlines-hidden -std=c++11 -fno-rtti -fno-exceptions -emit-llvm -S string_number_conversions_unittest.cc -o string_number_conversions_unittest.ll

3. Attach the two .ll to this report (so that we have a common baseline independent of libc++ etc. headers).

The .ll file should look the same with or without my patch. However you should get a valid/invalid executable when you run them through codegen with/without my patch. Commands to do that should look like this:

$ SYSROOT="$(xcrun --sdk iphoneos --show-sdk-path)"
$ clang++ -arch arm64 -S string_number_conversions.ll -o string_number_conversions.s
$ clang++ -arch arm64 -S string_number_conversions_unittest.ll -o string_number_conversions_unittest.s
$ clang++ -Xlinker -rpath -Xlinker @​executable_path/Frameworks -Xlinker -objc_abi_version -Xlinker 2 -arch arm64 -Werror -Wl,-dead_strip -isysroot ${SYSROOT} -stdlib=libc++ -miphoneos-version-min=9.0 -Wl,-ObjC -o base_unittests string_number_conversions.s string_number_conversions_unittest.s

as requested in comment 16
thakis-macpro:newdir thakis$ mv ../snc.cc .
thakis-macpro:newdir thakis$ mv ../sncu.cc .
thakis-macpro:newdir thakis$ export SYSROOT="$(xcrun --sdk iphoneos --show-sdk-path)"
thakis-macpro:newdir thakis$ ~/src/llvm-build2/bin/clang++ -DV8_DEPRECATION_WARNINGS -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=289944-1 -DCR_XCODE_VERSION=0810 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../.. -Igen -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -fcolor-diagnostics -arch arm64 -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Os -gdwarf-2 -isysroot ${SYSROOT} -stdlib=libc++ -miphoneos-version-min=9.0 -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -fno-threadsafe-statics -fvisibility-inlines-hidden -std=c++11 -fno-rtti -fno-exceptions -emit-llvm -S snc.cc -o string_number_conversions.ll
thakis-macpro:newdir thakis$ ~/src/llvm-build2/bin/clang++ -DV8_DEPRECATION_WARNINGS -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=289944-1 -DCR_XCODE_VERSION=0810 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../.. -Igen -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -fcolor-diagnostics -arch arm64 -Wall -Werror -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-undefined-var-template -Wno-nonportable-include-path -Wno-address-of-packed-member -Os -gdwarf-2 -isysroot ${SYSROOT} -stdlib=libc++ -miphoneos-version-min=9.0 -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -fno-threadsafe-statics -fvisibility-inlines-hidden -std=c++11 -fno-rtti -fno-exceptions -emit-llvm -S sncu.cc -o string_number_conversions_unittest.ll

thakis-macpro:newdir thakis$ ls -lh
total 1760
-rw-r--r-- 1 thakis eng 1.6K Jan 6 10:55 snc.cc
-rw-r--r-- 1 thakis eng 984B Jan 6 10:55 sncu.cc
-rw-r--r-- 1 thakis eng 501K Jan 6 10:56 string_number_conversions.ll
-rw-r--r-- 1 thakis eng 366K Jan 6 10:57 string_number_conversions_unittest.ll
thakis-macpro:newdir thakis$ zip files.zip *.ll
adding: string_number_conversions.ll (deflated 88%)
adding: string_number_conversions_unittest.ll (deflated 87%)

Good news, I can repro you not being able to repro :-P I can attach those compiled files if you think it's helpful, but I'm guessing it won't be. I can attach a good and a bad "real" binary if you think it's useful (it might be). I'll try to figure out why this repros in our full build but not with the simple commands you pasted. Will report back later today.

If I change your commands to a) produce .o files instead of .s files b) include more of the compilery flags from the compile commands I pasted, it seems to repro for me. With the .ll files I attached:

$ ~/src/llvm-build2/bin//clang++ -c --param=ssp-buffer-size=4 -fstack-protector -fcolor-diagnostics -arch arm64 -Os -gdwarf-2 -isysroot ${SYSROOT} -stdlib=libc++ -miphoneos-version-min=9.0 -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -fno-threadsafe-statics -fvisibility-inlines-hidden -std=c++11 -fno-rtti -fno-exceptions string_number_conversions_unittest.ll -o string_number_conversions_unittest.o
$ ~/src/llvm-build2/bin//clang++ -c --param=ssp-buffer-size=4 -fstack-protector -fcolor-diagnostics -arch arm64 -Os -gdwarf-2 -isysroot ${SYSROOT} -stdlib=libc++ -miphoneos-version-min=9.0 -fvisibility=hidden -Wheader-hygiene -Wstring-conversion -Wtautological-overlap-compare -fno-threadsafe-statics -fvisibility-inlines-hidden -std=c++11 -fno-rtti -fno-exceptions string_number_conversions.ll -o string_number_conversions.o
$ ~/src/llvm-build2/bin/clang++ -Xlinker -rpath -Xlinker @​executable_path/Frameworks -Xlinker -objc_abi_version -Xlinker 2 -arch arm64 -Werror -Wl,-dead_strip -isysroot ${SYSROOT} -stdlib=libc++ -miphoneos-version-min=9.0 -Wl,-ObjC -o base_unittests string_number_conversions.o string_number_conversions_unittest.o -framework UIKit -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation
$ otool -tV base_unittests | grep -C 10 nop | head -20
0000000100007a30 str xzr, [x8, #​688]
0000000100007a34 str xzr, [x8, #​680]
0000000100007a38 str xzr, [x8, #​672]
0000000100007a3c orr w9, wzr, #​0x10
0000000100007a40 strb w9, [x8, #​695]
0000000100007a44 adrp x9, 256 ; 0x100007000
0000000100007a48 add x9, x9, #​3947 ; literal pool for: "0123456789ABCDEF"
0000000100007a4c ldr q0, [x9]
0000000100007a50 str q0, [x8, #​672]
0000000100007a54 strb wzr, [x8, #​688]
0000000100007a58 nop <- XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX should be an adrp
0000000100007a5c add x9, x9, #​3964
0000000100007a60 str x9, [x8, #​696]
0000000100007a64 str x11, [x8, #​704]
0000000100007a68 strb w20, [x8, #​712]
0000000100007a6c movz w9, #​0x16
0000000100007a70 strb w9, [x8, #​743]
0000000100007a74 adrp x9, 256 ; 0x100007000
0000000100007a78 add x9, x9, #​3973 ; literal pool for: "0123456789ABCDEF012345"
0000000100007a7c add x10, x8, #​734

Thanks for reducing the testcase. I was able to reproduce and fix the issue. Committed again as r291266. Please re-open if there are further problems.

Our bot cycled green this time. Thanks!