[PATCH] D30770: Ensure that prefix data is preserved with subsections-via-symbols

[Peter Collingbourne via llvm-commits]

I can't seem to reproduce this on my Mac.

$ ~/src/llvm-project/ra/bin/clang -c lib.ll -o lib.o
warning: overriding the module target triple with
x86_64-apple-macosx10.12.0 [-Woverride-module]
1 warning generated.
$ ~/src/llvm-project/ra/bin/clang dont.ll lib.o -o dont -Wl,-dead_strip
warning: overriding the module target triple with
x86_64-apple-macosx10.12.0 [-Woverride-module]
1 warning generated.
$ ./dont
$ echo $?
123
$ cat lib.ll
  define void @f() prefix i32 123 {
   ret void
  }

$ cat dont.ll
  declare void @f();

  define i32 @main() {
   %f = bitcast void ()* @f to i32*
   %prefix_ptr = getelementptr inbounds i32, i32* %f, i32 -1
   %prefix_val = load i32, i32* %prefix_ptr
   ret i32 %prefix_val
  }

Which version of ld64 are you using? (I'm using 278.4 here.) I'd make sure
that it isn't too old to support .alt_entry.

In any event, I'm not sure if your patch is correct. In addition to the
points you made, declaring the prefix symbol like that could lead to symbol
conflicts if a function with prefix data has internal linkage and has the
same name as another function with internal linkage in another object file.

Peter

On Fri, Jul 28, 2017 at 6:29 AM, Moritz Angermann via Phabricator <
reviews at reviews.llvm.org> wrote:

> angerman added a comment.
>
> The following diff, seems to at least resolve this:
>
>   diff --git a/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
> b/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
>   index ff427c9a0d7..0aca2478cc5 100644
>   --- a/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
>   +++ b/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
>   @@ -655,25 +655,6 @@ void AsmPrinter::EmitFunctionHeader() {
>        OutStreamer->GetCommentOS() << '\n';
>      }
>
>   -  // Emit the prefix data.
>   -  if (F->hasPrefixData()) {
>   -    if (MAI->hasSubsectionsViaSymbols()) {
>   -      // Preserving prefix data on platforms which use
> subsections-via-symbols
>   -      // is a bit tricky. Here we introduce a symbol for the prefix data
>   -      // and use the .alt_entry attribute to mark the function's real
> entry point
>   -      // as an alternative entry point to the prefix-data symbol.
>   -      MCSymbol *PrefixSym = OutContext.createLinkerPrivateTempSymbol();
>   -      OutStreamer->EmitLabel(PrefixSym);
>   -
>   -      EmitGlobalConstant(F->getParent()->getDataLayout(),
> F->getPrefixData());
>   -
>   -      // Emit an .alt_entry directive for the actual function symbol.
>   -      OutStreamer->EmitSymbolAttribute(CurrentFnSym, MCSA_AltEntry);
>   -    } else {
>   -      EmitGlobalConstant(F->getParent()->getDataLayout(),
> F->getPrefixData());
>   -    }
>   -  }
>   -
>      // Emit the CurrentFnSym.  This is a virtual function to allow
> targets to
>      // do their wild and crazy things as required.
>      EmitFunctionEntryLabel();
>   @@ -725,6 +706,30 @@ void AsmPrinter::EmitFunctionEntryLabel() {
>        report_fatal_error("'" + Twine(CurrentFnSym->getName()) +
>                           "' label emitted multiple times to assembly
> file");
>
>   +  // Emit the prefix data.
>   +  const Function *F = MF->getFunction();
>   +  if (F->hasPrefixData()) {
>   +    if (MAI->hasSubsectionsViaSymbols()) {
>   +      // Preserving prefix data on platforms which use
> subsections-via-symbols
>   +      // is a bit tricky. Here we introduce a symbol for the prefix data
>   +      // and use the .alt_entry attribute to mark the function's real
> entry point
>   +      // as an alternative entry point to the prefix-data symbol.
>   +
>   +      MCSymbol *PrefixSym = OutContext.getOrCreateSymbol(
>   +          Twine(CurrentFnSym->getName()) + "$prefix");
>   +      PrefixSym->setPrivateExtern(true);
>   +
>   +      OutStreamer->EmitLabel(PrefixSym);
>   +
>   +      EmitGlobalConstant(F->getParent()->getDataLayout(),
> F->getPrefixData());
>   +
>   +      // Emit an .alt_entry directive for the actual function symbol.
>   +      OutStreamer->EmitSymbolAttribute(CurrentFnSym, MCSA_AltEntry);
>   +    } else {
>   +      EmitGlobalConstant(F->getParent()->getDataLayout(),
> F->getPrefixData());
>   +    }
>   +  }
>   +
>      return OutStreamer->EmitLabel(CurrentFnSym);
>    }
>
> There are however a few items I'm a bit unhappy about:
>
> - the prefix data is moved into the `EmitFunctionEntryLabel` from the
> `EmitFunctionHeader` function; this feels a little off.
> - this however ensures that `CurrentFnSym->redefineIfPossible();` is
> called before `CurrentFnSym->getName()` is used.
> - a `$prefix` is appended, which might if one decides to define `@f` with
> prefix data **and** `@f$prefix` clash.
> - The symbol is now set to private extern, and I'm not sure if this is the
> sweet spot we are looking for.
>
> @pcc if you could spare some feedback on this, that would be great. I'm
> truly sorry for having missed this case initially.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D30770
>
>
>
>


-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170728/97e7fb16/attachment.html>