Updated the patch based on review suggestion.

As suggested, I have switched back to what was my original change to fix this issue.

In D57960#1390773, @ruiu wrote:

This class should do a minimum number of attempt to remove a file, instead of trying to remove a file at various points to increase the chance to successfully removing the file. Why don't you close the file before removing the file?

Update to the eh-frame-negative-pcrel-sdata8.s test.

Updated testing as per review comments.

In D49557#1168549, @ruiu wrote:

Thanks! How did yoy find this?

Abandoned in favour of D44923.

Updated to address review comments.

In D44898#1048789, @ruiu wrote:

2.8%. Somehow clang-fsds got 2.39% faster.

That speedup is interesting. I don't have a good theory to explain it though.

Even if this patch is somewhat performance-neutral, we still should avoid extra memory allocation if we can. Let's try ignoring synthetic sections in ICF. That's I think better.

In D44898#1048688, @ruiu wrote:

How much did Scylla get slower?

Maybe the best option is just disabling ICF of synthetic sections?

That sounds like a better way of fixing the issue.

Updated to fix issue with clang compilation.

Removed assertions and tidied up white space a little.

Updated test to address review comments.

In D44898#1048263, @espindola wrote:

Seems reasonable. I will just do a quick check on the performance impact.

I think the PLT writing should handle the padding itself. It certainly feels more correct. It looks though D44775 is heading in that direction too.

In D44682#1043455, @ruiu wrote:

Instead of doing this, we should fill executable segments with trap instructions even if linker script is in use, shouldn't we?

Looking at the EhFrameSection writing and finalize code, it would seem that
some kind of alignment is being applied. This aligning is also the reason
why for x86_64 the mapping of CIE/FDE's to the output is so fragmented,
because all input we have seen is 4-byte aligned, but LLD uses 8-byte
alignment for the output.

Apart from the typo and the value for the alignment, LGTM.

Abandoning this revision in favour of D44168.

There are also some slight differences in the output between this implementation and the one in D44168. This patch does not output the "+0x0" for a zero offset and outputs the input section's alignment for the "Align" column. Which style of output is preferred?

Updated to address review comments.

I agree that this change is simpler, but it also doesn't produce the same output as D42960. Part of the reason for the "complexity" in my change in D42960 was to try to prevent the map file output from getting too big/verbose.

Updated to address comments.

Rebased and updated with minor change to make the output offset hex rather than decimal. Modified existing map file test to demonstrate the proposed .eh_frame output.

In D40366#933070, @zturner wrote:

I absolutely do not want to disable support for ConcRT under any circumstances unless we find that it is a completely broken library, which I highly doubt. Even then, I would want to ask Microsoft to fix it.

What is the exact problem you're running into?

In D40119#927529, @grimar wrote:

I think this made few bots be unhappy:
http://lab.llvm.org:8011/builders/clang-with-thin-lto-ubuntu/builds/6726
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/3069

Workaround no longer required. This issue is also fixed by commit r315932.

Committed in rL315292: [LLD] Fix findOrphanPos to consistently ignore "dead" OutputSection's.

Abandoned in favour of change https://reviews.llvm.org/D38571.

In D38539#889005, @ruiu wrote:

Can you try https://reviews.llvm.org/D38571 to see if it will fix your problem? Thanks.

What is the problem? std::thread::detach is a part of C++11, and it is hard to believe that we shouldn't use it at all.

In D38539#888333, @ruiu wrote:

From my understanding of FileOutputBuffer::commit in its current form, it does a call to sys::fs::rename for a regular file. So there is no direct association to the "unlink" performance issue described in the comment to unlinkAsync. Although, it could be that the implementation of rename on Linux could suffer from similar performance problems (i.e. it effectively still performs an unlink). Previously, FileOutputBuffer::create did remove (unlink) the target file. I believe that you recently removed this behaviour and it was this unlink that the unlinkAsync was trying to improve.

If you try it yourself, you'd realize that rename(2)'ing onto an existing file is as slow as calling unlink(2) on that file. My previous change to FileOutputBuffer doesn't fix the correctness issue in the usual use case, but it also improved the speed of lld, because lld no longer tries to unlink a file in the main thread. Before making that change, it tried to do that in the Driver by tryCreateFile.

Hi Rui,

Updated based on suggestion. I still think that keeping the common "AlignSize" is better and safer.

A similar fix was required for the string table, see https://reviews.llvm.org/D36267.

Is this patch good to land?

This patch fixes a Windows Visual Studio build lit issue introduced by rL306895 (D34855).

In D34855#798200, @grimar wrote:

Ah, I just noticed the D34853, it fixes the issue I am observing.

I believe that this "build mode" is intended for the Visual Studio MSVC build. This build is special in that it can produce builds for multiple configurations, e.g. Debug, Release & RelWithDebInfo, within the same top level build output directory. It is this configuration type that defines the "build mode". This means that the unit tests will only pick up the configuration that matches that of the lit that was run. Without the "build mode" I believe lit might end up running all configurations of unit tests that have been built, which is probably not the intended behaviour.

In D34204#790836, @andrewng wrote:

Hi Rafael,

Is this patch now good to land?

Cheers,
Andrew

Hi Rafael,

Updated for review comments.

Is there any more feedback on this patch or is it good to land?

In D34204#786240, @ruiu wrote:

Can you change your program?

In D34204#784448, @ruiu wrote:

Hi Andrew,

What are you actually trying to address with this patch? I'd like to know more about the background as to why you want this. Is there any reason you can't fix your program rather than the linker?

Is this patch good to land?

In D32910#747152, @zturner wrote:

I don't disbelieve you, but do you have any thoughts about why I've never seen this when running tests locally?

As suggested by Adrian, I have made prependDIExpr a static member function of DIExpression (and also appendOffset used by prependDIExpr). This is then used to apply the LEA address shift to the replacement debug value.

Sorry for the slight delay but only just got back from holiday.

In D31755#731245, @aprantl wrote:

Rather than scanning forward for DBG_VALUEs, is there a more systematic way to find them? E.g., by iterating over USEs? If not, doing it this way is probably fine.

Updated test to focus only on the instcombine pass.

I've updated the comments to explicitly mention the MinGW-w64 case that encounters this issue and removed the unnecessary FunctionType related checks.

In D31806#722732, @efriedma wrote:

gcc transforms the given C code to a call to expf(), just like LLVM, but then it looks like their inliner works a bit differently so the generated code ends up with a call to expf() rather than an infinite loop. With your patch, LLVM also generates a call to expf(). This seems weird... does MinGW's C library actually have an expf() function?

In D31806#721631, @efriedma wrote:

Can you explain a little more how exactly we end up in this situation? Your example has undefined behavior according to the C standard.

Removed DWARF version checks as suggested by Adrian (thanks) and updated comments.

Thanks for the review.

I have now split this patch and updated this review to contain the first part which fixes the codegen issue but doesn't preserve the debug values. I will work on a separate patch which will attempt to preserve the debug values.

In D30835#699193, @andrewng wrote:

In D30835#697996, @andrewng wrote:

In D30835#697933, @dblaikie wrote:

Looks like this change does two things - fixes the "debug info affects optimization" and also does some work to improve optimized debug info quality? Might be worth separating this into two changes to make sure there's good test coverage/easier to review/etc?

I'm about to leave work now but I will look on Monday into whether separating this into two makes sense.

On further consideration, I think it makes sense to keep the patch as is because otherwise the first patch would be "lossy", i.e. the output would lose debug information. So in fixing the optimisation, ideally the debug information should be correct and preserved. However, if there are concerns with the "patching" of the debug information, then it might make sense to split the patch. What are your thoughts?

In D30835#697996, @andrewng wrote:

In D30835#697933, @dblaikie wrote:

Looks like this change does two things - fixes the "debug info affects optimization" and also does some work to improve optimized debug info quality? Might be worth separating this into two changes to make sure there's good test coverage/easier to review/etc?

I'm about to leave work now but I will look on Monday into whether separating this into two makes sense.

In D30835#697933, @dblaikie wrote:

Looks like this change does two things - fixes the "debug info affects optimization" and also does some work to improve optimized debug info quality? Might be worth separating this into two changes to make sure there's good test coverage/easier to review/etc?