I may be missing something about how the ICF code works, but whilst looking at a separate ICF performance issue, I noticed these oddities in the code. I haven't actually seen any issues myself but reading through the code, it feels like there could be issues. Although I suspect they would be highly unlikely or perhaps not possible in actual practice.

Update for review suggestion.

Update for review comments.

Update to address review comments.

In D87469#2267576, @grimar wrote:

I think this is fine approach in general. I wonder if it should/could be splitted into 2 patches though:
one for InputSectionDescription and one for SectionPattern?

Update to address review comments and suggestions.

Updated to address review comments and suggestions.

In D87468#2266310, @MaskRay wrote:

Can you demonstrate how the old code has overhead related to the input?

In D87468#2266228, @MaskRay wrote:

GlobPattern::matchOne has a fast path for '*'. Isn't it fast enough?

This change requires D87468.

This is a prerequisite of D87469.

In D87245#2259958, @MaskRay wrote:

Thanks for noting this issue! Were you profiling ld.lld?

Address review comments.

LGTM with that last one change.

That test is for widenPath and it does cover a similar situation (I know because I wrote it). The code you have changed relates to directory_iterator_construct which does not have coverage of the situation where the UTF16 length is less than the UTF8 length, i.e. the issue that you are fixing.

In D83321#2140620, @danix800 wrote:

Full context attached @andrewng .

The usual practice is to include the entire context in the diff. However, the change itself LGTM.

In D77750#1980538, @thakis wrote:

Can you take a look, please?

Add comment to explain the Windows deficiencies (one of many)...

Updated to address review comments.

Updated to address review comments.

In D77184#1960437, @thakis wrote:

grimar, andrewng: You both have checkout and build on different drives too, yes?

The following patch fixes my issues on Windows, but I haven't tested that it doesn't break anything else:

Ping.

Ping.

Updated to address review comments and suggestions.

In D74477#1875164, @amccarth wrote:

Just curious: Do we know that the paths stored in the archive are actually UTF-8 and not just MBCS using whatever code page the user had when they created the archive?

In D74477#1874772, @rupprecht wrote:

In D74477#1873998, @andrewng wrote:

In my modified version of widenPath...

I second grimar's request for an associated test that covers this behavior, but are you saying that the issue is your modified version of sys::path::widenPath breaks some llvm-ar tests, and switching to sys::windows::UTF8ToUTF16 will keep them passing? (If so, submitting w/o an additional test sounds fine to me)

Just to add a bit more context for this change, I have a patch in progress to fix Windows UNC path handling in widenPath and that's how I stumbled upon this usage in llvm-ar. In my modified version of widenPath, I use the length of the input as UTF-16 to decide on whether the path needs to be "expanded", which should reduce the scope for problems in this llvm-ar use case. However, it really isn't needed here and hence this patch.

In D74477#1872033, @grimar wrote:

This is not necessary for CompareStringOrdinal and could possibly even cause problems.

Perhaps I am not a good person to review this patch. Have a question though: by "cause problems" do you know something that can be wrapped into a test case?

It is generally not good practice to hold a lock whilst exiting, as this has the potential to cause deadlocks. I don't believe that this situation can occur currently with LLD. However, after some recent observations, it appears that on Windows, in particular when using the static run-time, the only guaranteed way to not intermittently crash on exit is to wait for all threads to terminate (see related review D70447). Doing so would enable this deadlock situation to occur.

Updated patch to address review comments.

Noticed this again whilst working on D72775. Appears to have a minor performance benefit of ~2% for the same two links with stack sizes mentioned in D72775.

In D72775#1822384, @MaskRay wrote:

It'd be great if you can provide the exact numbers.

Where * = with this patch
Run with a release build with assertions enabled on Windows 10 with AMD Ryzen 3900X (12C/24T).

Updated to address review comments.