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

Thu Jan 13 17:50:54 PST 2022

keith added a comment.

In D117250#3242329 <https://reviews.llvm.org/D117250#3242329>, @oontvoo wrote:

> 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.

I agree with this but it's possible, especially w/o a compiler warning for this, some folks might see this as idealistic given with the standard linker it's never surfaced.

> Also, we can always fall back to setting this de-dup flag if the bug is in code that's not fixable, no?

Yes but I think for new users it's nicer to have 1:1 (as much as is realistic) behavior with ld64 to start, and then they can start investigating flags for performance. Versus finding some runtime misbehavior (hopefully folks wouldn't just give up at this point) and then having to find the flag to fix it.

>> 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.

I searched around a bit for this as well, any idea if there's a previous conversation about it anywhere? You get the warning for `foo == "a"` but not `foo == bar`, I wonder how folks would feel about adding one for that case. I also tested with ubsan and it doesn't warn for this either.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117250