In D104272#2820814, @dblaikie wrote:

In D104272#2820748, @aeubanks wrote:

I couldn't parse your comment, could you rephrase?

Oh, I just went and looked more - and now I see that the mangled parameter types are . separated, so the ambiguities I thought could occur (is p0i8 an opaque pointer followed by an i8 or a typed pointer-to-i8? Not actually ambiguous, because opaque pointer followed by i8 would be p0.i8 instead).

Carry on!

In D104272#2820587, @dblaikie wrote:

The patch looks mostly good, but I'm not sold on "op". Can we just use "p"?

Presumably then we wouldn't know whether to parse a type after the address space or not? Unless we only support this when everything is opaque pointers? (so there can be no confusion between opaque pointers or not)

update.

Address comment.

Rebase.

Update.

Update.

Add a test case.

address comment

Any update? Do you plan to land those two patches?

Refactor.

Address comment.

lgtm.

In D100966#2705542, @MaskRay wrote:

This is not correct. A symbol marked by .addrsig_sym can be referenced via normal means. With this patch, the diagnostic may change from error: undefined symbol: bar to error: relocation against symbol in discarded section: bar

Sorry, I'm not familiar with the format of coverage mapping.

• Check e.sym for null. If the flag /export=foo is provided and foo is not in the object file. e.sym will be null.
• Remove $ in exported name to have same name on windows and linux.

update and tested to build chrome.

Update testcase.

• Copy function attributes and remove ret/param attributes.
• Add a test case.

Can you add a test case for the llvm-cov new flag? Otherwise looks good to me.

In D99695#2664808, @phosek wrote:

In D99695#2664701, @vsk wrote:

I think then you would somehow need multiple -compilation-dir options if e.g. you have paths with different levels of ../ nesting, e.g. ../../foo.c and ../../../../bar.c. That sounds like it could become hard to manage.

In RawCoverageFilenamesReader::readUncompressed, we do:

SmallString<256> P(CWD);
llvm::sys::path::append(P, Filename);

Can this just be turned into an absolute path?

The flag would override the compilation dir that's stored inside the profile (the 0th path entry) so there should be just one. In that code snippet, it means overriding CWD.

In D99695#2664701, @vsk wrote:

I think then you would somehow need multiple -compilation-dir options if e.g. you have paths with different levels of ../ nesting, e.g. ../../foo.c and ../../../../bar.c. That sounds like it could become hard to manage.

This case is hard to manage. This patch would only handle case that all paths have same level of ../.

In D99695#2664545, @vsk wrote:

For example, if we have /tmp/src/a.cpp embedded in the binary, llvm-cov show -path-equivalence=/tmp/src/,/cov ... will generate output direcotry like this: coverage/cov/a.cpp.

What happens if the binary has a relative path '../../a.cpp' embedded and the user passes -path-equivalence=/tmp/src/,/cov to llvm-cov? Is a.cpp written to <coverage-dir>/cov/../../a.cpp?

In this case, ../../ is not matching /tmp/src. So, it's like -path-equivalence is not given. llvm-cov will lookup a.cpp in ../../ relative to current working directory.

This overlaps with the functionality of attribute used.

Update LangRef.

Split clang change and IR change.

LGTM.

Update test.

In D97101#2603321, @thakis wrote:

This broke ContinuousSyncMode/basic.c in check-profile on macOS (see e.g. end of https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8853690069583264896/+/steps/package_clang/0/stdout?format=raw), so I reverted it for now.

It repros on my laptop, so let me know if you want me to try things :)

In D97436#2601752, @thakis wrote:

I'm super confused by this change. lld-link used to do aggressive icf for code and safe icf for data. The consensus on https://bugs.chromium.org/p/chromium/issues/detail?id=1087923 was that we want aggressive icf for both code and data, like lld on other platforms. But this change adds an option to make ICF less aggressive, not more aggressive, from what I understand (correct?)

Address comment.

Thanks for reviewing.

use itanium abi in terminate-statements.cpp

• Deprecate deferred region. Use the notion of gap region inserted in VisitStmt to replace deferred region.
• Emit gap region with OutCounter from last statement if exists.
• Move test cases from deferred-region.cpp to terminate-statements.cpp

address comment.

Update test case.

Preserve the bahavior of /opt:icf. Using /opt:safeicf to do safe icf for all sections.

In D97436#2591761, @pcc wrote:

As far as I know the current behavior is that /opt:icf gives you full ICF for executable sections and safe ICF for non-executable sections. This is more than what MSVC does, which is full ICF for executable sections and no ICF for non-executable sections. I think it makes sense to add a mode that gives you full ICF for non-executable sections, but the behavior of /opt:icf should be preserved.

• Add /OPT:safeicf, which will mark all sections as unique if .addrsig doesn't exist. By default, /OPT:icf will do aggressive icf, matching the behavior of MSVC linker's /OPT:icf.
• Update test cases.