commit 9a8ed4a84d13db4bda0df7e508143cba36cf10ab
author Devin Jeanpierre <jeanpierreda@google.com> Wed Nov 22 15:57:30 2023 -0800
committer Copybara-Service <copybara-worker@google.com> Wed Nov 22 15:58:49 2023 -0800
tree a0c8e57fffa4eabb41377daa17e2c9f2f0d2c920
parent 0507300cda7ddb88f0e144c7ee81d3f1ee631ea7

Move bazel label suffix logic to Rust / `src_code_gen.rs`.

I also slightly tweaked the logic -- the bazel label suffix no longer crashes for empty bazel labels, or bazel labels that begin with a number. (Not that these ever exist!)

---

Background:

When a template class is instantiated, the function needs to have an added suffix, so that we do not generate multiple functions with the same symbol name. See [this doc](https://github.com/google/crubit/blob/main/docs/thunks_for_class_template_member_functions.md) for details.

The existing logic was fine and worked, but, it could not be invoked from Rust (needed for the linked bug) -- we need to use this mangled name when generating new C++ functions, not just for existing C++ functions.

Also, I will add, the existing logic was "action at a distance". It reused the `mangled_name` field, which normally refers to the actual symbol to link against, to add a suffix to it when it knows that we won't later try to link against it. So the code in file A was only correct due to code very far away in file B.

This code performed the addition of the suffix, in `importers/function.cc`:

```
  if (is_member_or_descendant_of_class_template) {
    // `thunks_for_class_template_member_functions.md` explains in more detail
    // why the `mangled_name` has to include the target name when working with
    // members or descendants of a class template.
    mangled_name += '_';
    mangled_name += ConvertToCcIdentifier(ictx_.GetOwningTarget(function_decl));
```

This would cause linking errors, since that mangled name now refers to a function that does not exist, except for this line of code in `src_code_gen.rs` (function `can_skip_cc_thunk`):

```
    if func.is_member_or_descendant_of_class_template {
        return false;
    }
```

Which causes (in a couple of different places) for a thunk to be generated, instead of linking to an existing thing with that `mangled_name`.

I would argue that this is fragile, or at least somewhat subtle/confusing. After this CL, since the logic is all in `src_code_gen.rs`, this goes away. We now _directly_ control the function name, entirely separately from the mangled name which is used for linking purposes. And so the suffix is added directly onto the thunk ident. Indeed, after this CL, we could use the suffix unconditionally for _all_ functions, if we so chose, removing the branch.

(And I would argue that we should indeed broaden the set of functions we add the suffix for. For example, if two headers define an (identical) inline function, e.g. due to textual headers with an inline function definition, the current code will cause an ODR violation, but an unconditional thunk rename would not. Being unconditional will just cause fewer bugs, I suspect. But, for now, this CL replicates the existing behavior closely.)

PiperOrigin-RevId: 584738431
Change-Id: I36820bca82ce58b47105416fb0256f6729db9ec2