[lld] [LLD][COFF] Implement support for hybrid IAT on ARM64X (PR #124189)

[Jacek Caban via llvm-commits]

================
@@ -717,52 +717,155 @@ class ExportOrdinalChunk : public NonSectionChunk {
 void IdataContents::create(COFFLinkerContext &ctx) {
   std::vector<std::vector<DefinedImportData *>> v = binImports(ctx, imports);
 
+  // Merge compatible EC and native import files in hybrid images.
+  if (ctx.hybridSymtab) {
+    for (std::vector<DefinedImportData *> &syms : v) {
+      // At this point, symbols are sorted by base name, ensuring that
+      // compatible import files, if present, are adjacent.
+      std::vector<DefinedImportData *> hybridSyms;
+      ImportFile *prev = nullptr;
+      for (DefinedImportData *sym : syms) {
+        ImportFile *file = sym->file;
+        if (!prev || file->isEC() == prev->isEC() ||
+            !file->isSameImport(prev)) {
+          hybridSyms.push_back(sym);
+          prev = file;
+          continue;
+        }
+
+        // The native variant exposes a subset of EC symbols and chunks. Use the
+        // EC variant to represent both.
+        if (file->isEC()) {
+          hybridSyms.pop_back();
----------------
cjacek wrote:

> Hmm, I feel unsure about this logic here, or about the assumptions it makes. Can you add more comments about how the logic of this loop works, and the assumptions? The comments currently do explain the highlevel 
goals of it, but I don't feel confident in the dedup logic.

I'll try to improve these comments.

> If there are both native and EC imports, are they ordered so that the native one comes first - is this assumed and guaranteed?

I don't think `binImports` guarantees this; the order may vary. That's why I check `isEC` on the new symbol and swap it with the one stored in `hybridSyms` if the native symbol appears first.

> I think what I may have a hard time wrapping my head around, is when the loop is written with the early-continue style (which may be policy and anyway generally is good structure) - the codepath we have here is essentially the special case, for `prev && file->isEC() != prev->isEC() && file->isSameImport(prev)`, i.e. deduplication. With that bit clarified for me, I have an easier time to understand the rest of the loop.

Yes, I can change the code to avoid using an early continue if you prefer.

> Is the check for `isEC()` required for that aspect of the deduplication? Having `isSameImport()` be true while `file->isEC() == prev->isEC()` seems unexpected. Or is it only included as a quicker way to decide this case without needing to do string comparisons?

I think it's possible for `isSameImport` to return true for two EC or two native symbols in cases like this:
```
func1
func2 EXPORTAS func1
```
Since the export name is the same, `isSameImport()` will return true, and they might appear next to each other. I'll add a test for this scenario.

https://github.com/llvm/llvm-project/pull/124189