> On 2014-Jul-24, at 13:13, Alex L <[email protected]> wrote: > > 4. For the "data" variables, I'm not convinced that changing `external` > variables to `private` is correct. Consider how variables get > merged between translation units. If you have a symbol that's > `weak` in one and `external` (strong) in another, references to the > weak symbol will point at the strong one instead. If you change the > `global` one to `private`, the references to the weak symbol won't > get updated. This doesn't seem like the right behaviour. > > Since the "data" variables are in `llvm.used`, I think we'll end up > with two copies of them (one `weak`, the other `private`). They'll > both point at counters, and it won't be clear which is canonical. > > I haven't considered this point before, but this seems to be a real concern. > Unfortunately this will mean that the size impact of private linkage will be > very small. > > The thing is that the `weak` and `external` symbols can represent functions > with different > source code, and I think for that case it makes sense not to merge the > profile data for > the `weak` function with the profile data for the `external` function. > Therefore I believe > that assigning `private` instead of `external` should work. > > However, right now when the raw profile data ends up with two records for the > function > with the same name, the profile data tool isn't able to handle the merging of > them correctly > as the hashes get mismatched because the functions have different source > code. > Thus the profile data tool has to be updated to handle this case in order for > this patch to be correct.
Hmm. I'm not sure what the right answer is here. Anyone else have an opinion? In the short therm, there's a somewhat boring blocker: the profile data format can't store multiple functions that have the same name (unless they have local linkage). I believe counters from one of them will be chosen arbitrarily. In practice this comes up rarely (e.g., `main()` from two different executables if their profiles get merged), but if we stopped merging "data" variables it would be more common. Even if `private` linkage even on "data" variables is the right direction, I think this needs to be solved first. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
