ARM-software / acle

Arm C Language Extensions (ACLE)
Other
89 stars 54 forks source link

[FMV] Change the default versions name mangling. #277

Closed DanielKristofKiss closed 8 months ago

DanielKristofKiss commented 11 months ago

The default version's name to be mangled to avoid problems with ifuncs due to the ifunc to be called as the base name of the function.

As for any pull request, please make sure to go through the below checklist.

Checklist: (mark with X those which apply)

vhscampos commented 11 months ago

Some points aren't clear in the text:

  1. Will this new default name mangling happen only with FMV enabled or globally?
  2. If the only version specified is the default one, either explicitly with the attribute or implictly without the attribute, will the new mangling still take place?
DanielKristofKiss commented 11 months ago

Some points aren't clear in the text:

  1. Will this new default name mangling happen only with FMV enabled or globally?

Only when FMV enabled. By amending the default version with "default" the basename could be used by the resolved symbol, referenced externally as it won't collide with the default function.

  1. If the only version specified is the default one, either explicitly with the attribute or implictly without the attribute, will the new mangling still take place?

Depends on, for the same as above reasons. if the toolchain can avoid emitting redirections then no need to use the default version. maybe a alias could be useful.

andrewcarlotti commented 10 months ago

I don't understand the remark about aliases in this change. Is this saying that the compiler can optimise versions with identical function bodies to a single function with additional aliased names? If so, I don't think that's relevant to this spec. Alternatively, is it referring to a case where a non-default version specifies only features that the compiler is already assuming to be present?

andrewcarlotti commented 10 months ago

Aside from that, I think the mangling changes are reasonable. It might be worth adding an explicit remark that the versioned symbol (i.e. the one that is resolved to one of the versions) should have the same mangling as would be used if the function didn't use function multiversioning.

DanielKristofKiss commented 10 months ago

I don't understand the remark about aliases in this change. Is this saying that the compiler can optimise versions with identical function bodies to a single function with additional aliased names? If so, I don't think that's relevant to this spec. Alternatively, is it referring to a case where a non-default version specifies only features that the compiler is already assuming to be present?

The intention is to be sure all function names remain always available when the certain optimisation eliminates a version. (e.g. the default and a given version got merged).

Aside from that, I think the mangling changes are reasonable. It might be worth adding an explicit remark that the versioned symbol (i.e. the one that is resolved to one of the versions) should have the same mangling as would be used if the function didn't use function multiversioning.

ACK.

andrewcarlotti commented 8 months ago

This looks good to me now. I've already implemented this mangling in GCC.