In D86000#2219322, @jfb wrote:

In D86000#2219288, @vsk wrote:

It'd be nice to fold the new check into an existing sanitizer group to bring this to a wider audience. Do you foresee adoption issues for existing -fsanitize=integer adopters? Fwiw some recently-added implicit conversion checks were folded in without much/any pushback.

integer does "not actually UB checks", right? I can certainly put it in there if you think I won't get yelled at 😄

In D86000#2219322, @jfb wrote:

In D86000#2219288, @vsk wrote:

It'd be nice to fold the new check into an existing sanitizer group to bring this to a wider audience. Do you foresee adoption issues for existing -fsanitize=integer adopters? Fwiw some recently-added implicit conversion checks were folded in without much/any pushback.

integer does "not actually UB checks", right? I can certainly put it in there if you think I won't get yelled at 😄

Emit a nop instruction to always mark the end of the MSan prologue, and insert prologue instructions before that.

Sorry, I noticed one typo just after I submitted the previous revised patch. It is corrected in this patch.

• Make workaround conditional to msvc (requested by @klausler)

So there is a lot of noise in this patch that is just reformatting on code that hasn't changed. It would be nice to get rid of any changes that are whitespace/indentation/formatting only. Also see inlined comments for needed changes.

Here is the final document with Chris Lattner's suggestions. I have also included index.rst, which now refers to this document rather than the two old ones.

LGTM, thanks

In D85937#2219318, @Meinersbur wrote:

In D85937#2219266, @klausler wrote:

But are there other workarounds? Would adding "static" to the original "constexpr" suffice to avoid the bug?

No.

In D86000#2219288, @vsk wrote:

It'd be nice to fold the new check into an existing sanitizer group to bring this to a wider audience. Do you foresee adoption issues for existing -fsanitize=integer adopters? Fwiw some recently-added implicit conversion checks were folded in without much/any pushback.

In D85937#2219266, @klausler wrote:

But are there other workarounds? Would adding "static" to the original "constexpr" suffice to avoid the bug?

address comments

Addressed everyone's comments as well as the clang tidy warnings.

Thanks for working on this. I think making lldMain "reentrancy safe" is useful.

In D85937#2219151, @DavidTruby wrote:

In D85937#2219052, @Meinersbur wrote:

In D85937#2219028, @klausler wrote:

You might have to use conditional preprocessing to make this workaround specific to MSVC.

The workaround is valid C++ and works fine with every compiler. So why add a #if maze?

I also believe it will generate exactly the same code. There's no functional difference between the new proposal and the old code as far as the standard is concerned. I agree that preprocessor changes aren't necessary here.

It'd be nice to fold the new check into an existing sanitizer group to bring this to a wider audience. Do you foresee adoption issues for existing -fsanitize=integer adopters? Fwiw some recently-added implicit conversion checks were folded in without much/any pushback.

This adds future implementation complexity.

Fix clang-(format|tidy)

In D85937#2219052, @Meinersbur wrote:

In D85937#2219028, @klausler wrote:

You might have to use conditional preprocessing to make this workaround specific to MSVC.

The workaround is valid C++ and works fine with every compiler. So why add a #if maze?

Caught by http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/41009/steps/ninja%20check%201/logs/FAIL%3A%20MemorySanitizer-POWERPC64%3A%3A%20chained_origin_memcpy.cpp

lgtm

In D85996#2219124, @lebedev.ri wrote:

1. I'm pretty sure it's not okay to allocate memory in crash handler.
2. We can't know that the internal state is still consistent.

i agree with both points but all i was trying to do i salvage what can be salvaged.

Very nice improvement to tests!

Refactor to share code directly with asan.

Like I mentioned on the review for the prototype, I still think we should try to implement a scheme that makes CK_BItCast between fixed and scalable types trivial. Doing coercion this way is going to have a significant performance cost. But there isn't any user-visible effect, so I'm fine with leaving that for a followup.

Add support for testing LLD as a library, by running the driver several times in a row when LLD_IN_TEST is defined. The value of LLD_IN_TEST tells how many times LLD should loop (3 times right now).

Thank you

LGTM

LGTM

LGTM

This seems to fix a problem.

• Fix without passing the NodeBuilder

In D79097#2218765, @hnrklssn wrote:

In D79097#2218587, @anemet wrote:

In D79097#2218567, @hnrklssn wrote:

In D79097#2218463, @anemet wrote:

@hnrklssn hi, are you panning to get back to this at some point?

Yes, sorry about the delay!
Currently finishing the last bits of the thesis that this work is part of has taken priority, which doesn't leave too much time since I'm now also working full time.
After that's done though I intend to come back and do the cleanup necessary to upstream as much of the work as possible. It's all related to improving optimisation remarks in one way or another. :)

Oh cool, are you planning to publish/publicize this work once it's done?

Yup, it will be published in LUP Student Papers afaict! I can send you a rough draft if you're interested. Most of the final content is in place already, but it's a bit rough around the edges.

LGTM++

ping

In D85937#2219052, @Meinersbur wrote:

In D85937#2219028, @klausler wrote:

You might have to use conditional preprocessing to make this workaround specific to MSVC.

The workaround is valid C++ and works fine with every compiler. So why add a #if maze?

LGTM

In D83507#2219127, @Tyker wrote:

In D83507#2215586, @lebedev.ri wrote:

Please rebase and update the affected test :)

thank you, but how did you reduced it ?

Remove unneeded braces.

CC @aaron.ballman who committed Features.def for signing off or knowing someone who can sign off.

In D83507#2215586, @lebedev.ri wrote:

Please rebase and update the affected test :)

1. I'm pretty sure it's not okay to allcate memory in crash handler.
2. We can't know that the internal state is still consistent.

This looks good to me. Note that I was actually suggesting to avoid the dynamic_lookup + forward declare and requiring that the *SDK* is at least 10.14 to run tests (which is separate from your OS version). But this way it's even more compatible, so I think we should go with it.

Can you also add a test for the standard paths not being searched without a syslibroot prefix? As in, if you pass -syslibroot and there's e.g. no /usr/lib under it, we shouldn't add the standard /usr/lib to the search path.

Move removeUnreachableBlocks before inserting the prologue. This makes sure there's no issue with the ActualFnStart instruction being deleted before the visitor loop.

LGTM.