[PATCH] D117250: [lld-macho] Mention string literal deduplication as a difference from ld64

Thu Jan 13 17:22:47 PST 2022

oontvoo added a comment.

In D117250#3242211 <https://reviews.llvm.org/D117250#3242211>, @keith wrote:

> In D117250#3242166 <https://reviews.llvm.org/D117250#3242166>, @oontvoo wrote:
>
>>> Specifically the proliferation of closed source libraries for analytics / fraud / payments are very common, and might have these subtle bugs that developers cannot easily fix. Really I think flipping the default would just be a benefit for folks who were onboarding.
>>
>> Can you elaborate what kind of bugs  *not* deduping literals by default  could lead to?
>
> I mean I guess literally anything right? The case I found for us was luckily a crasher (repro here https://github.com/llvm/llvm-project/issues/51822) but given some logic error who knows I guess?
> It's definitely true that you would be more likely to introduce bugs in your own code with deduping on, but in the closed source binary case, where the producers of these aren't likely to be using lld themselves, us finding those bugs downstream isn't super helpful. 
> Tbh even if we did find it I'm not   sure all vendors would be receptive to bugs that only reproduce with the non-default linker, but that's tbd if we can actually even spot them given they could be much more subtle logic errors. I

I can appreciate the complexity+frustration  in chasing and fixing bugs like this, having done that for a few dozens of projects in our code base.
But this is clearly UB (both C/C++ and ObjC never guarantee pointer equality for strings with the same values).

I don't see the argument here being about speed (even though in practice, it kind of is). To me, it's more about whether LLD should make it easy for people to rely on UB - and I'm really not convinced that it should.
Also, we can always fall back to setting this de-dup flag if the bug is in code that's not fixable, no?

> It's too bad that there isn't a clang warning for this case AFAICT.

IIRC, there is only one complier-warning for this kind of thing in  ObjC - and I've recently added a clang-tidy check for another scenario -  but yeah, that's about it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117250