agozillon added a comment.

In D149162#4404379 <https://reviews.llvm.org/D149162#4404379>, @fmayer wrote:

> In D149162#4404271 <https://reviews.llvm.org/D149162#4404271>, @agozillon 
> wrote:
>
>> I have hopefully fixed the sanitizer issue (the incorrect assert, thank you 
>> again for the catch) with the following patch: 
>> https://github.com/llvm/llvm-project/commit/309023263dba3b02bc885101faa47d110f662f99
>>  it was a slightly more involved change than I expected, I had to rework the 
>> getTargetEntryUniqueInfo function to use a callback function to support 
>> Clang's use of a fallback filename and line number (gathered from a 
>> PresumedLoc). So I apologies for the extended time to land the change.
>
> Thanks! I saw you use `llvm_unreachable` for error handling. Is that actually 
> unreachable by construction of the program? Otherwise the compiler is free to 
> assume this is never reached and will trigger undefined behaviour if it is.

I was actually unaware of that behavior, thank you very much for pointing it 
out!

The condition is reachable, but it's a condition that will result in incorrect 
kernel name generation in the resulting IR, so I imagine an assert is more apt 
then, I will push essentially the same line that's in this patch but inverted 
to be the right condition for it to trigger, i.e. !EC. Just running some local 
tests first to make sure all is fine. If you have another suggestion for the 
type of error construct to be used please do mention it however! I was 
considering report_fatal_error, but that appears to be for cases where 
compilation cannot continue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149162/new/

https://reviews.llvm.org/D149162

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to