ABataev added a comment. In D108421#2962199 <https://reviews.llvm.org/D108421#2962199>, @lebedev.ri wrote:
> In D108421#2962160 <https://reviews.llvm.org/D108421#2962160>, @ABataev wrote: > >> In D108421#2962151 <https://reviews.llvm.org/D108421#2962151>, @lebedev.ri >> wrote: >> >>> In D108421#2961611 <https://reviews.llvm.org/D108421#2961611>, >>> @kamleshbhalui wrote: >>> >>>> updated test and make changes local to auto generated global vars for lock. >>> >>> I do not understand why the most original diff here was wrong. >>> Is is the wrong default for `getOrCreateInternalVariable()` to default to >>> `dso_local`? >> >> Need to check that the linkage/visibility of the created vars is not >> changing in place. If it is not changing, then it is good to go. Otherwise, >> need to add some another function/extra parameter with linkage/visibility >> settings. > > I'm having trouble parsing the comment, but i don't really see why the fact > that some users may be changing linkage > should deter us from having the right default. Yes, but the fix should not introduce bugs, if possible. Need to check the users and make them work correctly too, that's why need to check the users and if some of them change the linkage/visibility, then need to add a new function or add an extra parameter to track whether the need to add `dso_local` attribute. > If some users change linkage, then regardless of where this is dealt with, > the users will have to be adjusted, just a different set of them. It seems to > me that fixing the ones that already mess with linkage is the right choice, > not fixing the ones that *don't* mess with linkage. That's all I'm asking, actually. Or just keep the original code (if no time to go through the users) and fix `dso_local` only where required. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108421/new/ https://reviews.llvm.org/D108421 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
