rajendrant, does this ring a bell? (If not, don't worry too much, might be a toolchain bug, and it's on an experimental bot)

hans: I wonder if this is due to LTO?

That bot doesn't do LTO builds (it's not setting is_official_build), so it must be something else

Sorry. I missed this bug.
I do not see any possible issue.

Repros in a local release component build, but not in a debug component build. Probably some ICF thing similar to  bug 635943 ?

Here's the problem. DataUseMeasurementTest calls SetUserData() on a request either with key UserRequestUserDAtaForTesting::kUserDataKey or with data_use_measurement::DataUseUserDataKey::kUserDataKey. If the former key is used, that request is considered a "user request", else it's considered a "service request".

The first constant is defined like so:

class UserRequestUserDataForTesting : public base::SupportsUserData::Data,
                                      public URLRequestClassifier {
 public:
  static const void* const kUserDataKey;
...
};

// static
const void* const UserRequestUserDataForTesting::kUserDataKey =
    &UserRequestUserDataForTesting::kUserDataKey;



The second constant:

class DataUseUserData : public base::SupportsUserData::Data {
 public:
...
  static const void* const kUserDataKey;
};

// static
const void* const DataUseUserData::kUserDataKey =
    &DataUseUserData::kUserDataKey;

(https://cs.chromium.org/chromium/src/components/data_use_measurement/core/data_use_user_data.cc?sq=package:chromium&dr=CSs&rcl=1481559468&l=37)


It looks like lld ICFs these two kUserDataKeys together. Changing the first kUserDataKeyDefinition to

const int kRandom = 42;
const void* const UserRequestUserDataForTesting::kUserDataKey = &kRandom;

makes the test pass.



Merging these feels a bit aggressive to me, and bugs like this are probably difficult to diagnose for people who haven't seen this type of bug before. Should I just land the change I described above? Do we feel good with people having to debug something like this every now and then in the future?

I think from the point of view of ICF, doing this makes sense, but I guess that most people expect that ICF merges only functions, so we probably should only functions.

https://codereview.chromium.org/2565173005/

This is the second time I've seen LLD's ICF of readonly data break user code, so I think we need to power it down. There's a reason link.exe doesn't do this.

Do we have any numbers on how much ICF'ing ro data saves?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/501fb6dbe391965bd11219475fcb1980491312f4

commit 501fb6dbe391965bd11219475fcb1980491312f4
Author: thakis <thakis@chromium.org>
Date: Tue Dec 13 01:23:35 2016

Make DataUseMeasurementTest.UserNotUserTest pass with lld/win.

BUG= 668207 
TBR=rnk

Review-Url: https://codereview.chromium.org/2565173005
Cr-Commit-Position: refs/heads/master@{#437977}

[modify] https://crrev.com/501fb6dbe391965bd11219475fcb1980491312f4/components/data_use_measurement/core/data_use_measurement_unittest.cc

https://build.chromium.org/p/chromium.fyi/builders/CrWinClangLLD64%20tester \o/