[PATCH] D50101: [asan] Update a vector's storage annotation during destruction.

[Ben via Phabricator via cfe-commits]

bobsayshilol added a comment.

Pinging this as it's been "Ready to Land" for a while and I want to check that 
that's not because I've forgotten to do something?


Repository:
  rCXX libc++

https://reviews.llvm.org/D50101



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50101: [asan] Update a vector's storage annotation during destruction.

[Ben via Phabricator via cfe-commits]

bobsayshilol created this revision.
bobsayshilol added reviewers: mclow.lists, EricWF.
Herald added subscribers: cfe-commits, ldionne.

This allows a user replaced operator delete to modify or reuse returned memory 
in the case where the size and capacity of a vector do not match upon 
destruction and hence leave a portion of the memory poisoned.
This consequently also balances __annotate_delete() calls with those of 
__annotate_new().


Repository:
  rCXX libc++

https://reviews.llvm.org/D50101

Files:
  include/vector


Index: include/vector
===
--- include/vector
+++ include/vector
@@ -540,13 +540,14 @@
 value_type,
 typename 
iterator_traits<_ForwardIterator>::reference>::value>::type* = 0);
 
-#if _LIBCPP_DEBUG_LEVEL >= 2
 _LIBCPP_INLINE_VISIBILITY
 ~vector()
 {
+__annotate_delete();
+#if _LIBCPP_DEBUG_LEVEL >= 2
 __get_db()->__erase_c(this);
-}
 #endif
+}
 
 vector(const vector& __x);
 vector(const vector& __x, const allocator_type& __a);


Index: include/vector
===
--- include/vector
+++ include/vector
@@ -540,13 +540,14 @@
 value_type,
 typename iterator_traits<_ForwardIterator>::reference>::value>::type* = 0);
 
-#if _LIBCPP_DEBUG_LEVEL >= 2
 _LIBCPP_INLINE_VISIBILITY
 ~vector()
 {
+__annotate_delete();
+#if _LIBCPP_DEBUG_LEVEL >= 2
 __get_db()->__erase_c(this);
-}
 #endif
+}
 
 vector(const vector& __x);
 vector(const vector& __x, const allocator_type& __a);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50101: [asan] Update a vector's storage annotation during destruction.

[Ben via Phabricator via cfe-commits]

bobsayshilol added a comment.

Thanks! I don't have write access to svn so I can't commit the patch myself, 
but assuming that I've read the docs correctly if anyone is able to take it I'd 
be grateful.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50101



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53263: [CodeGen] Fix places where the return type of a FunctionDecl was being used in place of the function type

[Ben via Phabricator via cfe-commits]

bobsayshilol created this revision.
bobsayshilol added reviewers: sberg, ahatanak.
Herald added subscribers: cfe-commits, jfb.

`FunctionDecl::Create()` takes as its `T` parameter the type of function that 
should be created, not the return type. Passing in the return type looks to 
have been copypasta'd around a bit, but the number of correct usages outweighs 
the incorrect ones so I've opted for keeping what `T` is the same and fixing up 
the call sites instead.

This fixes a crash in Clang when attempting to compile the following snippet of 
code with `-fblocks -fsanitize=function -x objective-c++` (the original repro 
case):

  void g(void(^)());
  void f()
  {
__block int a = 0;
g(^(){ a++; });
  }

as well as the following which only requires `-fsanitize=function -x c++`:

  void f(char * buf)
  {
__builtin_os_log_format(buf, "");
  }

I haven't added the above as tests or checked that all of the current tests 
build so I don't expect this to get merged straight away, but I wanted to check 
that this is matching guidelines and potentially avoiding any duplicate work if 
someone else also starts trying to track down this issue too.


Repository:
  rC Clang

https://reviews.llvm.org/D53263

Files:
  lib/AST/Decl.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CGObjC.cpp
  lib/CodeGen/CGStmtOpenMP.cpp
  lib/CodeGen/ItaniumCXXABI.cpp

Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -2315,11 +2315,13 @@
 FTy, GlobalInitFnName, getTypes().arrangeNullaryFunction(),
 SourceLocation());
 ASTContext &Ctx = getContext();
+QualType ReturnTy = Ctx.VoidTy;
+QualType FunctionTy = Ctx.getFunctionType(ReturnTy, llvm::None, {});
 FunctionDecl *FD = FunctionDecl::Create(
 Ctx, Ctx.getTranslationUnitDecl(), SourceLocation(), SourceLocation(),
-&Ctx.Idents.get(GlobalInitFnName), Ctx.VoidTy, nullptr, SC_Static,
+&Ctx.Idents.get(GlobalInitFnName), FunctionTy, nullptr, SC_Static,
 false, false);
-CGF.StartFunction(GlobalDecl(FD), getContext().VoidTy, GlobalInitFn,
+CGF.StartFunction(GlobalDecl(FD), ReturnTy, GlobalInitFn,
   getTypes().arrangeNullaryFunction(), FunctionArgList(),
   SourceLocation(), SourceLocation());
 
Index: lib/CodeGen/CGStmtOpenMP.cpp
===
--- lib/CodeGen/CGStmtOpenMP.cpp
+++ lib/CodeGen/CGStmtOpenMP.cpp
@@ -385,11 +385,11 @@
   FunctionDecl *DebugFunctionDecl = nullptr;
   if (!FO.UIntPtrCastRequired) {
 FunctionProtoType::ExtProtoInfo EPI;
+QualType FunctionTy = Ctx.getFunctionType(Ctx.VoidTy, llvm::None, EPI);
 DebugFunctionDecl = FunctionDecl::Create(
 Ctx, Ctx.getTranslationUnitDecl(), FO.S->getBeginLoc(),
-SourceLocation(), DeclarationName(), Ctx.VoidTy,
-Ctx.getTrivialTypeSourceInfo(
-Ctx.getFunctionType(Ctx.VoidTy, llvm::None, EPI)),
+SourceLocation(), DeclarationName(), FunctionTy,
+Ctx.getTrivialTypeSourceInfo(FunctionTy),
 SC_Static, /*isInlineSpecified=*/false, /*hasWrittenPrototype=*/false);
   }
   for (const FieldDecl *FD : RD->fields()) {
Index: lib/CodeGen/CGObjC.cpp
===
--- lib/CodeGen/CGObjC.cpp
+++ lib/CodeGen/CGObjC.cpp
@@ -3229,29 +3229,36 @@
   ASTContext &C = getContext();
   IdentifierInfo *II
 = &CGM.getContext().Idents.get("__assign_helper_atomic_property_");
+
+  QualType ReturnTy = C.VoidTy;
+  QualType DestTy = C.getPointerType(Ty);
+  QualType SrcTy = Ty;
+  SrcTy.addConst();
+  SrcTy = C.getPointerType(SrcTy);
+
+  SmallVector ArgTys;
+  ArgTys.push_back(DestTy);
+  ArgTys.push_back(SrcTy);
+  QualType FunctionTy = C.getFunctionType(ReturnTy, ArgTys, {});
+
   FunctionDecl *FD = FunctionDecl::Create(C,
   C.getTranslationUnitDecl(),
   SourceLocation(),
-  SourceLocation(), II, C.VoidTy,
+  SourceLocation(), II, FunctionTy,
   nullptr, SC_Static,
   false,
   false);
 
-  QualType DestTy = C.getPointerType(Ty);
-  QualType SrcTy = Ty;
-  SrcTy.addConst();
-  SrcTy = C.getPointerType(SrcTy);
-
   FunctionArgList args;
-  ImplicitParamDecl DstDecl(getContext(), FD, SourceLocation(), /*Id=*/nullptr,
+  ImplicitParamDecl DstDecl(C, FD, SourceLocation(), /*Id=*/nullptr,
 DestTy, ImplicitParamDecl::Other);
   args.push_back(&DstDecl);
-  ImplicitParamDecl SrcDecl(getContext(), FD, SourceLocation(), /*Id=*/nullptr,
+  ImplicitParamDecl SrcDecl(C, FD, SourceLocation(), 

[PATCH] D53263: [CodeGen] Fix places where the return type of a FunctionDecl was being used in place of the function type

[Ben via Phabricator via cfe-commits]

bobsayshilol updated this revision to Diff 170136.
bobsayshilol edited the summary of this revision.
bobsayshilol added a comment.

Fixed the added assert to do the right thing. Clang can now build with a debug 
Clang built from this patch, and all unittests I could find pass in that built 
version too.


https://reviews.llvm.org/D53263

Files:
  lib/AST/Decl.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CGObjC.cpp
  lib/CodeGen/CGStmtOpenMP.cpp
  lib/CodeGen/ItaniumCXXABI.cpp

Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -2315,11 +2315,13 @@
 FTy, GlobalInitFnName, getTypes().arrangeNullaryFunction(),
 SourceLocation());
 ASTContext &Ctx = getContext();
+QualType ReturnTy = Ctx.VoidTy;
+QualType FunctionTy = Ctx.getFunctionType(ReturnTy, llvm::None, {});
 FunctionDecl *FD = FunctionDecl::Create(
 Ctx, Ctx.getTranslationUnitDecl(), SourceLocation(), SourceLocation(),
-&Ctx.Idents.get(GlobalInitFnName), Ctx.VoidTy, nullptr, SC_Static,
+&Ctx.Idents.get(GlobalInitFnName), FunctionTy, nullptr, SC_Static,
 false, false);
-CGF.StartFunction(GlobalDecl(FD), getContext().VoidTy, GlobalInitFn,
+CGF.StartFunction(GlobalDecl(FD), ReturnTy, GlobalInitFn,
   getTypes().arrangeNullaryFunction(), FunctionArgList(),
   SourceLocation(), SourceLocation());
 
Index: lib/CodeGen/CGStmtOpenMP.cpp
===
--- lib/CodeGen/CGStmtOpenMP.cpp
+++ lib/CodeGen/CGStmtOpenMP.cpp
@@ -385,11 +385,11 @@
   FunctionDecl *DebugFunctionDecl = nullptr;
   if (!FO.UIntPtrCastRequired) {
 FunctionProtoType::ExtProtoInfo EPI;
+QualType FunctionTy = Ctx.getFunctionType(Ctx.VoidTy, llvm::None, EPI);
 DebugFunctionDecl = FunctionDecl::Create(
 Ctx, Ctx.getTranslationUnitDecl(), FO.S->getBeginLoc(),
-SourceLocation(), DeclarationName(), Ctx.VoidTy,
-Ctx.getTrivialTypeSourceInfo(
-Ctx.getFunctionType(Ctx.VoidTy, llvm::None, EPI)),
+SourceLocation(), DeclarationName(), FunctionTy,
+Ctx.getTrivialTypeSourceInfo(FunctionTy),
 SC_Static, /*isInlineSpecified=*/false, /*hasWrittenPrototype=*/false);
   }
   for (const FieldDecl *FD : RD->fields()) {
Index: lib/CodeGen/CGObjC.cpp
===
--- lib/CodeGen/CGObjC.cpp
+++ lib/CodeGen/CGObjC.cpp
@@ -3229,29 +3229,36 @@
   ASTContext &C = getContext();
   IdentifierInfo *II
 = &CGM.getContext().Idents.get("__assign_helper_atomic_property_");
+
+  QualType ReturnTy = C.VoidTy;
+  QualType DestTy = C.getPointerType(Ty);
+  QualType SrcTy = Ty;
+  SrcTy.addConst();
+  SrcTy = C.getPointerType(SrcTy);
+
+  SmallVector ArgTys;
+  ArgTys.push_back(DestTy);
+  ArgTys.push_back(SrcTy);
+  QualType FunctionTy = C.getFunctionType(ReturnTy, ArgTys, {});
+
   FunctionDecl *FD = FunctionDecl::Create(C,
   C.getTranslationUnitDecl(),
   SourceLocation(),
-  SourceLocation(), II, C.VoidTy,
+  SourceLocation(), II, FunctionTy,
   nullptr, SC_Static,
   false,
   false);
 
-  QualType DestTy = C.getPointerType(Ty);
-  QualType SrcTy = Ty;
-  SrcTy.addConst();
-  SrcTy = C.getPointerType(SrcTy);
-
   FunctionArgList args;
-  ImplicitParamDecl DstDecl(getContext(), FD, SourceLocation(), /*Id=*/nullptr,
+  ImplicitParamDecl DstDecl(C, FD, SourceLocation(), /*Id=*/nullptr,
 DestTy, ImplicitParamDecl::Other);
   args.push_back(&DstDecl);
-  ImplicitParamDecl SrcDecl(getContext(), FD, SourceLocation(), /*Id=*/nullptr,
+  ImplicitParamDecl SrcDecl(C, FD, SourceLocation(), /*Id=*/nullptr,
 SrcTy, ImplicitParamDecl::Other);
   args.push_back(&SrcDecl);
 
   const CGFunctionInfo &FI =
-CGM.getTypes().arrangeBuiltinFunctionDeclaration(C.VoidTy, args);
+CGM.getTypes().arrangeBuiltinFunctionDeclaration(ReturnTy, args);
 
   llvm::FunctionType *LTy = CGM.getTypes().GetFunctionType(FI);
 
@@ -3262,7 +3269,7 @@
 
   CGM.SetInternalFunctionAttributes(GlobalDecl(), Fn, FI);
 
-  StartFunction(FD, C.VoidTy, Fn, FI, args);
+  StartFunction(FD, ReturnTy, Fn, FI, args);
 
   DeclRefExpr DstExpr(&DstDecl, false, DestTy,
   VK_RValue, SourceLocation());
@@ -3301,50 +3308,56 @@
   if ((!(PD->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_atomic)))
 return nullptr;
   llvm::Constant *HelperFn = nullptr;
-
   if (hasTrivialGetExpr(PID))
 return nullptr;
   assert(PID->getGetterCXXConstructor()

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

[Ben via Phabricator via cfe-commits]

bobsayshilol updated this revision to Diff 170277.
bobsayshilol retitled this revision from "[CodeGen] Fix places where the return 
type of a FunctionDecl was being used in place of the function type" to "Fix 
places where the return type of a FunctionDecl was being used in place of the 
function type".
bobsayshilol added a comment.

Add missing part of previous change to patch. Now all unit tests pass.

I'm removing [CodeGen] from the title too since this change touches more than 
just that section now.


https://reviews.llvm.org/D53263

Files:
  lib/AST/Decl.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CGObjC.cpp
  lib/CodeGen/CGStmtOpenMP.cpp
  lib/CodeGen/ItaniumCXXABI.cpp
  lib/Frontend/Rewrite/RewriteModernObjC.cpp

Index: lib/Frontend/Rewrite/RewriteModernObjC.cpp
===
--- lib/Frontend/Rewrite/RewriteModernObjC.cpp
+++ lib/Frontend/Rewrite/RewriteModernObjC.cpp
@@ -3099,10 +3099,10 @@
  SmallVectorImpl &MsgExprs,
  ObjCMethodDecl *Method) {
   // Now do the "normal" pointer to function cast.
-  QualType castType = getSimpleFunctionType(returnType, ArgTypes,
+  QualType funcType = getSimpleFunctionType(returnType, ArgTypes,
 Method ? Method->isVariadic()
: false);
-  castType = Context->getPointerType(castType);
+  QualType castType = Context->getPointerType(funcType);
 
   // build type for containing the objc_msgSend_stret object.
   static unsigned stretCount=0;
@@ -3177,7 +3177,7 @@
   // AST for __Stretn(receiver, args).s;
   IdentifierInfo *ID = &Context->Idents.get(name);
   FunctionDecl *FD = FunctionDecl::Create(*Context, TUDecl, SourceLocation(),
-  SourceLocation(), ID, castType,
+  SourceLocation(), ID, funcType,
   nullptr, SC_Extern, false, false);
   DeclRefExpr *DRE = new (Context) DeclRefExpr(FD, false, castType, VK_RValue,
SourceLocation());
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -2315,11 +2315,13 @@
 FTy, GlobalInitFnName, getTypes().arrangeNullaryFunction(),
 SourceLocation());
 ASTContext &Ctx = getContext();
+QualType ReturnTy = Ctx.VoidTy;
+QualType FunctionTy = Ctx.getFunctionType(ReturnTy, llvm::None, {});
 FunctionDecl *FD = FunctionDecl::Create(
 Ctx, Ctx.getTranslationUnitDecl(), SourceLocation(), SourceLocation(),
-&Ctx.Idents.get(GlobalInitFnName), Ctx.VoidTy, nullptr, SC_Static,
+&Ctx.Idents.get(GlobalInitFnName), FunctionTy, nullptr, SC_Static,
 false, false);
-CGF.StartFunction(GlobalDecl(FD), getContext().VoidTy, GlobalInitFn,
+CGF.StartFunction(GlobalDecl(FD), ReturnTy, GlobalInitFn,
   getTypes().arrangeNullaryFunction(), FunctionArgList(),
   SourceLocation(), SourceLocation());
 
Index: lib/CodeGen/CGStmtOpenMP.cpp
===
--- lib/CodeGen/CGStmtOpenMP.cpp
+++ lib/CodeGen/CGStmtOpenMP.cpp
@@ -385,11 +385,11 @@
   FunctionDecl *DebugFunctionDecl = nullptr;
   if (!FO.UIntPtrCastRequired) {
 FunctionProtoType::ExtProtoInfo EPI;
+QualType FunctionTy = Ctx.getFunctionType(Ctx.VoidTy, llvm::None, EPI);
 DebugFunctionDecl = FunctionDecl::Create(
 Ctx, Ctx.getTranslationUnitDecl(), FO.S->getBeginLoc(),
-SourceLocation(), DeclarationName(), Ctx.VoidTy,
-Ctx.getTrivialTypeSourceInfo(
-Ctx.getFunctionType(Ctx.VoidTy, llvm::None, EPI)),
+SourceLocation(), DeclarationName(), FunctionTy,
+Ctx.getTrivialTypeSourceInfo(FunctionTy),
 SC_Static, /*isInlineSpecified=*/false, /*hasWrittenPrototype=*/false);
   }
   for (const FieldDecl *FD : RD->fields()) {
Index: lib/CodeGen/CGObjC.cpp
===
--- lib/CodeGen/CGObjC.cpp
+++ lib/CodeGen/CGObjC.cpp
@@ -3229,29 +3229,36 @@
   ASTContext &C = getContext();
   IdentifierInfo *II
 = &CGM.getContext().Idents.get("__assign_helper_atomic_property_");
+
+  QualType ReturnTy = C.VoidTy;
+  QualType DestTy = C.getPointerType(Ty);
+  QualType SrcTy = Ty;
+  SrcTy.addConst();
+  SrcTy = C.getPointerType(SrcTy);
+
+  SmallVector ArgTys;
+  ArgTys.push_back(DestTy);
+  ArgTys.push_back(SrcTy);
+  QualType FunctionTy = C.getFunctionType(ReturnTy, ArgTys, {});
+
   FunctionDecl *FD = FunctionDecl::Create(C,
   C.getTranslationUnitDecl(),
   SourceLocation(

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

[Ben via Phabricator via cfe-commits]

bobsayshilol added a comment.

Ping.


https://reviews.llvm.org/D53263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

[Ben via Phabricator via cfe-commits]

bobsayshilol added a reviewer: rjmccall.
bobsayshilol added a comment.

Ping.


https://reviews.llvm.org/D53263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

[Ben via Phabricator via cfe-commits]

bobsayshilol added a comment.

Thanks!
I don't have commit access to land this myself.


https://reviews.llvm.org/D53263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

[Ben via Phabricator via cfe-commits]

bobsayshilol added a comment.

In https://reviews.llvm.org/D53263#1294383, @kristina wrote:

> I can do it if you'd like, will be a moment though.


Thanks and much appreciated!


https://reviews.llvm.org/D53263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

[Ben via Phabricator via cfe-commits]

bobsayshilol added a comment.

In https://reviews.llvm.org/D53263#1294477, @kristina wrote:

> Huge apologies, it seems I can't get this to patch cleanly against my fork 
> and therefore can't test it before committing, which is something I generally 
> always do. I'll leave it to someone else. Again, huge apologies, hopefully 
> you won't have to wait too long.


No worries! Thanks anyway for taking a look.

In https://reviews.llvm.org/D53263#1294488, @JDevlieghere wrote:

> The patch applies for me but has quite a few style violations. I'll fix those 
> up before landing it. Also needs a test (I'll add the one from the 
> description).


Ah I was trying to match the code around it and didn't think to run clang-tidy 
so I'll keep that in mind next time, and I didn't add a new test because just 
the assert in `Decl.cpp` was enough to flag up misuses with `check-clang` but 
I'm definitely not against adding more tests. Thanks for landing!

Thanks again everyone!


Repository:
  rL LLVM

https://reviews.llvm.org/D53263



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60274: [ELF] Implement Dependent Libraries Feature

[ben via Phabricator via cfe-commits]

bd1976llvm marked 25 inline comments as done.
bd1976llvm added a comment.

I have updated the diff to address review comments. I think we can continue to 
refine this patch in parallel with discussing the design further (I am not 
dismissing the concerns raised by @compnerd and @jyknight).

I am unsure if I should be doing validation on the structure of the 
llvm.dependent-libraries named metadata e.g. what if there are zero operands or 
more than one operand for a metadata node. Does anyone know what  LLVM's policy 
is?




Comment at: lld/ELF/InputFiles.cpp:406
+  Driver->addFile(*S, /*WithLOption=*/true);
+else if (Optional S = searchLibrary(Specifier))
+  Driver->addFile(*S, /*WithLOption=*/true);

pcc wrote:
> `searchLibrary` contains the support for handling flags like `-l:foo`; I 
> guess if you don't want that behaviour here then it will need to be factored 
> out of `searchLibrary`.
I was undecided here. Refactoring the code to remove the colon prefix behavior 
and adding other checks like only allowing libraries (not objects) to be added 
to the link adds complexity that might not be justified. Maybe we should simply 
document that some behaviors might work but are unsupported and liable to 
change without warning?



Comment at: lld/ELF/InputFiles.cpp:657
+  CHECK(this->getObj().getSectionContents(&Sec), this);
+  for (const uint8_t *P = Contents.begin(), *E = Contents.end(); P < E;) {
+StringRef A = reinterpret_cast(P);

ruiu wrote:
> Instead of allowing multiple names in a single .autolink section, why don't 
> you create multiple .autolink sections?
This would simplify the implementation but I don't think that the size 
increases in the input files is worth it. Also, it is easier to dump the 
dependent libraries if they are all in one section.



Comment at: llvm/include/llvm/BinaryFormat/ELF.h:841
 // for safe ICF.
+  SHT_LLVM_DEPLIBS = 0x6fff4c04,// LLVM Dependent Library Specifiers.
   // Android's experimental support for SHT_RELR sections.

peter.smith wrote:
> pcc wrote:
> > peter.smith wrote:
> > > This is the same value as has been proposed in another ELF extension in 
> > > https://reviews.llvm.org/D60242 , May be worth coordinating here. It is 
> > > probably worth us having a central place to document the LLVM specific 
> > > ELF extensions as we are accumulating enough of them.
> > I don't have any existing dependencies on the value that I've chosen. If 
> > Ben has no dependencies of his own, I reckon that whoever ends up 
> > committing first can just get this value, and the next person will get the 
> > next one. I think this can be our general policy.
> > 
> > Do you think that https://llvm.org/docs/Extensions.html#elf-dependent is 
> > not sufficient as a place to document our extensions?
> I think it could be, now that I see it fully expanding with some of the 
> extensions mentioning the elf terminology. I think we should mention the 
> SHT_... and hex value so that someone doesn't have to dig for that in the 
> source code.
I think that it is better if the hex value is in this one place and we use the 
SHT_ token everywhere else. This reduces the potential for mistakes.



Comment at: llvm/include/llvm/Object/IRSymtab.h:157
+/// Dependent Library Specifiers
+  Range DepLibs;
 };

pcc wrote:
> Maybe just `Range`?
Thanks! I went for the struct with a single element representation because that 
is used for comdats. Should we make this same change for comdats?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60274/new/

https://reviews.llvm.org/D60274



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60274: [ELF] Implement Dependent Libraries Feature

[ben via Phabricator via cfe-commits]

bd1976llvm marked 5 inline comments as done.
bd1976llvm added inline comments.



Comment at: lld/ELF/InputFiles.cpp:402
+  if (Config->DepLibs)
+if (fs::exists(Specifier))
+  Driver->addFile(Specifier, /*WithLOption=*/false);

jyknight wrote:
> bd1976llvm wrote:
> > jyknight wrote:
> > > bd1976llvm wrote:
> > > > jyknight wrote:
> > > > > This bit seems unfortunate. "-lfoo" won't search for a file named 
> > > > > "foo", and having this code do so does not seem like a good idea. I'd 
> > > > > want this feature to work just like -l.
> > > > > 
> > > > > Actually, I think it'd be better to preserve the "-l" prefix in the 
> > > > > deplibs section. (I'd note that even if libs in the search path are 
> > > > > spelled "-lfoo", that doesn't mean we need accept any other options)
> > > > > 
> > > > > If we want to support loading a filename which not on the search path 
> > > > > (but...do we, even? That seems like a kinda scary feature to me...), 
> > > > > then it'd be obvious how to spell that: "libfoo.a" and 
> > > > > "/baz/bar/foo.a" would open files directly, vs "-lfoo" and "-l:foo.a" 
> > > > > would search for them on the search path.
> > > > What you  have described is the behavior of the INPUT linker script 
> > > > command:  
> > > > https://sourceware.org/binutils/docs/ld/File-Commands.html#File-Commands.
> > > > 
> > > > We have carefully designed this "dependent libraries" feature to avoid 
> > > > mapping "comment lib" pragmas to --library command line options. My 
> > > > reasons:
> > > > 
> > > > - I don't want the compiler doing string processing to try to satisfy 
> > > > the command line parsing behaviour of the target linker.
> > > > 
> > > > - I don't want to couple the source code to a GNU-style linker. After 
> > > > all there are other ELF linkers. Your proposal isn't merely an ELF 
> > > > linking proposal, it's a proposal for ELF toolchains with GNU-like 
> > > > linkers (e.g. the arm linker doesn't support the colon prefix 
> > > > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0474c/Cjahbdei.html).
> > > > 
> > > > - The syntax is #pragma comment(lib, ...) not #pragma 
> > > > linker-option(library, ...) i.e. the only thing this (frankly rather 
> > > > bizarre) syntax definitely implies is that the argument is related to 
> > > > libraries (and comments ¯\_(ツ)_/¯); it is a bit of a stretch to 
> > > > interpret "comment lib" pragmas as mapping directly to "specifying an 
> > > > additional --library command line option".
> > > > 
> > > > - I want to avoid GNUism's and use a "general" mechanism. MSVC source 
> > > > code compatibility is a usecase.
> > > > 
> > > > - It is important to support loading a filename which not on the search 
> > > > path. For example I have seen developers use the following: #pragma 
> > > > comment(lib, __FILE__"\\..\\foo.lib")
> > > > 
> > > > I would like the following to work on for ELF:
> > > > 
> > > > #pragma comment(lib, "foo") => add libfoo.a to the link.
> > > > #pragma comment(lib, "foo.lib") => add foo.lib to the link
> > > > #pragma comment(lib, "c:\\foo.lib") => add c:\foo.lib to the link
> > > > 
> > > > The way the code is now these will work on both LLD and MSVC (and I 
> > > > think it will work for Apple's linker as well although I haven't 
> > > > checked). In addition, we have been careful to come up with a design 
> > > > that can be implemented by the GNU linkers... with the cost that some 
> > > > complicated links will not be possible to do via autolinking; in these 
> > > > (IMO rare) cases developers will need to use the command line directly 
> > > > instead.
> > > > 
> > > > What I am trying to accomplish is to try to keep #pragma comment(lib, 
> > > > "foo") compatible across linkers. Otherwise we will be in a situation 
> > > > where you will have to have the equivalent of...
> > > > 
> > > > #ifdef COFF_LIBRARIES:
> > > > #pragma comment(lib, "/DEFAULTLIB:foo.lib")
> > > > #'elif ELF_LIBRARIES_GNU_STYLE_LINKER:
> > > > #pragma comment(lib, "-lfoo")
> > > > #'elif DARWIN_FRAMEWORKS:
> > > > #pragma comment(lib, "-framework foo")
> > > > #esle
> > > > #error Please specify linking model
> > > > #endif
> > > > 
> > > > ... in your source code (or the moral equivalent somewhere else). We 
> > > > can avoid this.
> > > > 
> > > > Also note that I am not proposing to remove the .linker-options 
> > > > extension. We can add support for .linker-options to LLD in the future 
> > > > if we find a usecase where developers want pragmas that map directly to 
> > > > the linkers command line options for ELF. If this is a usecase then, in 
> > > > the future, we can implement support for #pragma comment(linker, ) 
> > > > pragmas that lower to .linker-options; #pragma comment(lib, ...) 
> > > > pragmas can continue to lower to .deplibs.
> > > It just seems to me a really bad idea to have a situation where 
> > > specifying `#pragma comment(lib, "foo")` can either open a file

[PATCH] D60274: [ELF] Implement Dependent Libraries Feature

[ben via Phabricator via cfe-commits]

bd1976llvm updated this revision to Diff 195495.
bd1976llvm added a comment.

No longer shortening "dependent libraries" to "deplibs" except for the .deplibs 
section (as this takes up bytes on disk).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60274/new/

https://reviews.llvm.org/D60274

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/dependent-lib.c
  clang/test/CodeGen/elf-linker-options.c
  clang/test/CodeGen/pragma-comment.c
  clang/test/Modules/autolink.m
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/Driver.h
  lld/ELF/DriverUtils.cpp
  lld/ELF/InputFiles.cpp
  lld/ELF/Options.td
  lld/test/ELF/Inputs/deplibs-lib_bar.s
  lld/test/ELF/Inputs/deplibs-lib_foo.s
  lld/test/ELF/deplibs-colon-prefix.s
  lld/test/ELF/deplibs-corrupt.s
  lld/test/ELF/deplibs.s
  lld/test/ELF/lto/deplibs.s
  llvm/docs/Extensions.rst
  llvm/docs/LangRef.rst
  llvm/include/llvm-c/lto.h
  llvm/include/llvm/BinaryFormat/ELF.h
  llvm/include/llvm/LTO/LTO.h
  llvm/include/llvm/LTO/legacy/LTOModule.h
  llvm/include/llvm/Object/IRSymtab.h
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/LTO/LTO.cpp
  llvm/lib/LTO/LTOModule.cpp
  llvm/lib/MC/MCParser/ELFAsmParser.cpp
  llvm/lib/MC/MCSectionELF.cpp
  llvm/lib/Object/ELF.cpp
  llvm/lib/Object/IRSymtab.cpp
  llvm/lib/ObjectYAML/ELFYAML.cpp
  llvm/test/Feature/elf-deplibs.ll
  llvm/test/LTO/Resolution/X86/symtab-elf.ll
  llvm/test/MC/ELF/section.s
  llvm/test/Object/X86/irsymtab.ll
  llvm/tools/llvm-lto2/llvm-lto2.cpp
  llvm/tools/llvm-readobj/ELFDumper.cpp
  llvm/tools/lto/lto.cpp

Index: llvm/tools/lto/lto.cpp
===
--- llvm/tools/lto/lto.cpp
+++ llvm/tools/lto/lto.cpp
@@ -325,6 +325,10 @@
   return unwrap(mod)->getLinkerOpts().data();
 }
 
+const char* lto_module_get_dependent_libraries(lto_module_t mod) {
+return unwrap(mod)->getDependentLibraries().data();
+}
+
 void lto_codegen_set_diagnostic_handler(lto_code_gen_t cg,
 lto_diagnostic_handler_t diag_handler,
 void *ctxt) {
Index: llvm/tools/llvm-readobj/ELFDumper.cpp
===
--- llvm/tools/llvm-readobj/ELFDumper.cpp
+++ llvm/tools/llvm-readobj/ELFDumper.cpp
@@ -2830,6 +2830,8 @@
 return "LLVM_CALL_GRAPH_PROFILE";
   case SHT_LLVM_ADDRSIG:
 return "LLVM_ADDRSIG";
+  case SHT_LLVM_DEPENDENT_LIBRARIES:
+return "LLVM_DEPENDENT_LIBRARIES";
   // FIXME: Parse processor specific GNU attributes
   case SHT_GNU_ATTRIBUTES:
 return "ATTRIBUTES";
Index: llvm/tools/llvm-lto2/llvm-lto2.cpp
===
--- llvm/tools/llvm-lto2/llvm-lto2.cpp
+++ llvm/tools/llvm-lto2/llvm-lto2.cpp
@@ -360,6 +360,13 @@
 if (TT.isOSBinFormatCOFF())
   outs() << "linker opts: " << Input->getCOFFLinkerOpts() << '\n';
 
+if (TT.isOSBinFormatELF()) {
+  outs() << "dependent libraries:";
+  for (auto L : Input->getDependentLibraries())
+outs() << " \"" << L << "\"";
+  outs() << '\n';
+}
+
 std::vector ComdatTable = Input->getComdatTable();
 for (const InputFile::Symbol &Sym : Input->symbols()) {
   switch (Sym.getVisibility()) {
Index: llvm/test/Object/X86/irsymtab.ll
===
--- llvm/test/Object/X86/irsymtab.ll
+++ llvm/test/Object/X86/irsymtab.ll
@@ -9,16 +9,17 @@
 
 ; BCA:   blob data = '\x01\x00\x00\x00\x06\x00\x00\x00\x08\x00\x00\x00D\x00\x00\x00\x01\x00\x00\x00P\x00\x00\x00\x00\x00\x00\x00P\x00\x00\x00\x02\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00\x0E\x00\x00\x00\x18\x00\x00\x00&\x00\x00\x00\x0B\x00\x00\x001\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x00$\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x08$\x00\x00'
+; BCA-NEXT:blob data = '\x02\x00\x00\x00\x06\x00\x00\x00\x08\x00\x00\x00L\x00\x00\x00\x01\x00\x00\x00X\x00\x00\x00\x00\x00\x00\x00X\x00\x00\x00\x02\x00\x00\x00\x88\x00\x00\x00\x00\x00\x00\x00\x0E\x00\x00\x00\x18\x00\x00\x00&\x00\x00\x00\x0B\x00\x00\x001\x00\x00\x00\x00\x00\x00\x00\x88\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x00$\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x08$\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00'
 ; BCA-NEXT: 
 ; BCA-NEXT:  blob data = 'foobarproducerx86_64-unknown-linux-gnuirsymtab.ll'
 ; BCA-NEXT: 
 
-; SYMTAB:  version: 1
+; SYMTAB:  version: 2
 ; SYMTAB-NEXT: producer: producer
 ; SYMTAB-NEXT: target triple: x86_64-unknown-linux-gnu
 ; SYMTAB-NEXT: source filename: irsymtab.ll

[PATCH] D60274: [ELF] Implement Dependent Libraries Feature

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

I am keen to keep this moving.

I think there are a few things outstanding:

1. Need to resolve concerns w.r.t the design.
2. I need to find out whether I should be doing validation when reading the 
metadata in LLVM.
3. The LLD side needs a more detailed review.

@jyknight @compnerd can we  progress the design conversation?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60274/new/

https://reviews.llvm.org/D60274



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60274: [ELF] Implement Dependent Libraries Feature

[ben via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rLLD360984: [ELF] Implement Dependent Libraries Feature 
(authored by bd1976llvm, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60274?vs=195495&id=199972#toc

Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60274/new/

https://reviews.llvm.org/D60274

Files:
  ELF/Config.h
  ELF/Driver.cpp
  ELF/Driver.h
  ELF/DriverUtils.cpp
  ELF/InputFiles.cpp
  ELF/Options.td
  test/ELF/Inputs/deplibs-lib_bar.s
  test/ELF/Inputs/deplibs-lib_foo.s
  test/ELF/deplibs-colon-prefix.s
  test/ELF/deplibs-corrupt.s
  test/ELF/deplibs.s
  test/ELF/lto/deplibs.s

Index: ELF/Config.h
===
--- ELF/Config.h
+++ ELF/Config.h
@@ -137,6 +137,7 @@
   bool Cref;
   bool DefineCommon;
   bool Demangle = true;
+  bool DependentLibraries;
   bool DisableVerify;
   bool EhFrameHdr;
   bool EmitLLVM;
Index: ELF/Driver.cpp
===
--- ELF/Driver.cpp
+++ ELF/Driver.cpp
@@ -790,6 +790,7 @@
   Config->DefineCommon = Args.hasFlag(OPT_define_common, OPT_no_define_common,
   !Args.hasArg(OPT_relocatable));
   Config->Demangle = Args.hasFlag(OPT_demangle, OPT_no_demangle, true);
+  Config->DependentLibraries = Args.hasFlag(OPT_dependent_libraries, OPT_no_dependent_libraries, true);
   Config->DisableVerify = Args.hasArg(OPT_disable_verify);
   Config->Discard = getDiscard(Args);
   Config->DwoDir = Args.getLastArgValue(OPT_plugin_opt_dwo_dir_eq);
@@ -1548,9 +1549,11 @@
 Symtab->trace(Arg->getValue());
 
   // Add all files to the symbol table. This will add almost all
-  // symbols that we need to the symbol table.
-  for (InputFile *F : Files)
-parseFile(F);
+  // symbols that we need to the symbol table. This process might
+  // add files to the link, via autolinking, these files are always
+  // appended to the Files vector.
+  for (size_t I = 0; I < Files.size(); ++I)
+parseFile(Files[I]);
 
   // Now that we have every file, we can decide if we will need a
   // dynamic symbol table.
Index: ELF/InputFiles.cpp
===
--- ELF/InputFiles.cpp
+++ ELF/InputFiles.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "InputFiles.h"
+#include "Driver.h"
 #include "InputSection.h"
 #include "LinkerScript.h"
 #include "SymbolTable.h"
@@ -499,6 +500,27 @@
   }
 }
 
+// An ELF object file may contain a `.deplibs` section. If it exists, the
+// section contains a list of library specifiers such as `m` for libm. This
+// function resolves a given name by finding the first matching library checking
+// the various ways that a library can be specified to LLD. This ELF extension
+// is a form of autolinking and is called `dependent libraries`. It is currently
+// unique to LLVM and lld.
+static void addDependentLibrary(StringRef Specifier, const InputFile *F) {
+  if (!Config->DependentLibraries)
+return;
+  if (fs::exists(Specifier))
+Driver->addFile(Specifier, /*WithLOption=*/false);
+  else if (Optional S = findFromSearchPaths(Specifier))
+Driver->addFile(*S, /*WithLOption=*/true);
+  else if (Optional S = searchLibraryBaseName(Specifier))
+Driver->addFile(*S, /*WithLOption=*/true);
+  else
+error(toString(F) +
+  ": unable to find library from dependent library specifier: " +
+  Specifier);
+}
+
 template 
 void ObjFile::initializeSections(
 DenseSet &ComdatGroups) {
@@ -740,6 +762,24 @@
 }
 return &InputSection::Discarded;
   }
+  case SHT_LLVM_DEPENDENT_LIBRARIES: {
+if (Config->Relocatable)
+  break;
+ArrayRef Data =
+CHECK(this->getObj().template getSectionContentsAsArray(&Sec), this);
+if (!Data.empty() && Data.back() != '\0') {
+  error(toString(this) +
+": corrupted dependent libraries section (unterminated string): " +
+Name);
+  return &InputSection::Discarded;
+}
+for (const char *D = Data.begin(), *E = Data.end(); D < E;) {
+  StringRef S(D);
+  addDependentLibrary(S, this);
+  D += S.size() + 1;
+}
+return &InputSection::Discarded;
+  }
   case SHT_RELA:
   case SHT_REL: {
 // Find a relocation target section and associate this section with that.
@@ -1302,6 +1342,9 @@
 
   for (const lto::InputFile::Symbol &ObjSym : Obj->symbols())
 Symbols.push_back(createBitcodeSymbol(KeptComdats, ObjSym, *this));
+
+  for (auto L : Obj->getDependentLibraries())
+addDependentLibrary(L, this);
 }
 
 static ELFKind getELFKind(MemoryBufferRef MB, StringRef ArchiveName) {
Index: ELF/Options.td
===
--- ELF/Options.td
+++ ELF/Options.td
@@ -71,6 +71,10 @@
 "Apply link-time values for dynamic relocations",
 "Do not ap

[PATCH] D68147: [MC][ELF] Prevent globals with an explicit section from being mergeable by default and add a safe option to allow mergeable

[ben via Phabricator via cfe-commits]

bd1976llvm created this revision.
Herald added subscribers: llvm-commits, cfe-commits, hiraditya.
Herald added projects: clang, LLVM.

Alternative to https://reviews.llvm.org/D68101

Prevents globals with an explicit section from being mergeable by default (as 
in D68101 ).
Adds a command line option to allow the explicit section to be mergeable; but, 
an error will be emitted if broken output would be created.
Users can use the command line option if the default behaviour results in a 
performance regression.

This is a partial fix for https://bugs.llvm.org/show_bug.cgi?id=43457.


Repository:
  rC Clang

https://reviews.llvm.org/D68147

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/include/llvm/CodeGen/CommandFlags.inc
  llvm/include/llvm/Target/TargetMachine.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/Target/TargetLoweringObjectFile.cpp
  llvm/test/CodeGen/X86/explict-section-mergeable.ll
  llvm/test/CodeGen/X86/section_mergeable_size.ll

Index: llvm/test/CodeGen/X86/section_mergeable_size.ll
===
--- llvm/test/CodeGen/X86/section_mergeable_size.ll
+++ llvm/test/CodeGen/X86/section_mergeable_size.ll
@@ -1,3 +1,3 @@
-; RUN: llc -mtriple x86_64-linux-gnu < %s | FileCheck %s
+; RUN: llc -mtriple x86_64-linux-gnu -mergeable-explicit-sections < %s | FileCheck %s
 @a = internal unnamed_addr constant [1 x [1 x i32]] zeroinitializer, section ".init.rodata", align 4
 ; CHECK: .init.rodata,"aM",{{[@%]}}progbits,4
Index: llvm/test/CodeGen/X86/explict-section-mergeable.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/explict-section-mergeable.ll
@@ -0,0 +1,27 @@
+; RUN: llc < %s -mtriple=x86_64-linux-gnu | FileCheck %s --check-prefix=DEFAULT
+
+;; show that the implicitly generated sections are as expected
+@implicit1 = unnamed_addr constant [2 x i16] [i16 1, i16 1]
+; DEFAULT-CHECK: .section .rodata.cst4,"aM",@progbits,4
+@implicit2 = unnamed_addr constant [2 x i32] [i32 1, i32 1]
+; DEFAULT-CHECK: .section .rodata.cst8,"aM",@progbits,8
+@implicit3 = unnamed_addr constant [2 x i16] [i16 1, i16 0]
+; DEFAULT-CHECK: .section .rodata.str2.2,"aMS",@progbits,2
+
+;; Check that an explicitly created section is not SHF_MERGE and that
+;; it contains all three symbols.
+@explicit1 = unnamed_addr constant [2 x i16] [i16 1, i16 1] "rodata-section"=".explicit"
+@explicit2 = unnamed_addr constant [2 x i32] [i32 1, i32 1] "rodata-section"=".explicit"
+@explicit3 = unnamed_addr constant [2 x i16] [i16 1, i16 0] "rodata-section"=".explicit"
+; DEFAULT: .section .explicit,"a",@progbits
+; DEFAULT-NOT: .section
+; DEFAULT: explicit1:
+; DEFAULT-NOT: .section
+; DEFAULT: explicit2:
+; DEFAULT-NOT: .section
+; DEFAULT: explicit3:
+
+; RUN: not llc < %s -mtriple=x86_64-linux-gnu -mergeable-explicit-sections 2>&1 | FileCheck %s --check-prefix=MERGE
+
+; MERGE: Symbol 'explicit2' from module '' required a section with entry-size=8 but was placed in section '.explicit' with entry-size=4: Explicit assignment by pragma or attribute of incompatible symbols to a section?
+
Index: llvm/lib/Target/TargetLoweringObjectFile.cpp
===
--- llvm/lib/Target/TargetLoweringObjectFile.cpp
+++ llvm/lib/Target/TargetLoweringObjectFile.cpp
@@ -135,6 +135,26 @@
 const MCSymbol *Sym) const {
 }
 
+static bool inExplicitSection(const GlobalObject *GO, SectionKind Kind) {
+  if (GO->hasSection())
+return true;
+
+  if (auto *GVar = dyn_cast(GO)) {
+auto Attrs = GVar->getAttributes();
+if ((Attrs.hasAttribute("bss-section") && Kind.isBSS()) ||
+(Attrs.hasAttribute("data-section") && Kind.isData()) ||
+(Attrs.hasAttribute("rodata-section") && Kind.isReadOnly())) {
+  return true;
+}
+  }
+
+  if (auto *F = dyn_cast(GO)) {
+if (F->hasFnAttribute("implicit-section-name"))
+  return true;
+  }
+
+  return false;
+}
 
 /// getKindForGlobal - This is a top-level target-independent classifier for
 /// a global object.  Given a global variable and information from the TM, this
@@ -187,6 +207,16 @@
   if (!GVar->hasGlobalUnnamedAddr())
 return SectionKind::getReadOnly();
 
+  // If two globals with differing sizes end up in the same mergeable section that
+  // section can be assigned an incorrect entry size. To avoid this we do not put
+  // globals into a mergeable section if they have been explicitly assigned to a section.
+  // TODO: This is a pessimistic solution as we lose the benifits of mergeable sections. A
+  //   better solution migh

[PATCH] D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes

[ben via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG86478d3de91a: [MC][ELF] Put explicit section name symbols 
into entry size compatible sections (authored by bd1976llvm).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72194/new/

https://reviews.llvm.org/D72194

Files:
  clang/test/CodeGen/cfstring-elf-sections-x86_64.c
  llvm/include/llvm/IR/DiagnosticInfo.h
  llvm/include/llvm/MC/MCContext.h
  llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
  llvm/lib/MC/MCContext.cpp
  llvm/test/CodeGen/X86/explicit-section-mergeable.ll
  llvm/unittests/ExecutionEngine/Orc/LegacyRTDyldObjectLinkingLayerTest.cpp
  llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp

Index: llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp
===
--- llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp
+++ llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp
@@ -74,10 +74,12 @@
   LLVMContext Context;
   auto M = std::make_unique("", Context);
   M->setTargetTriple("x86_64-unknown-linux-gnu");
-  Type *Int32Ty = IntegerType::get(Context, 32);
-  GlobalVariable *GV =
-  new GlobalVariable(*M, Int32Ty, false, GlobalValue::ExternalLinkage,
- ConstantInt::get(Int32Ty, 42), "foo");
+  Constant *StrConstant = ConstantDataArray::getString(Context, "forty-two");
+  auto *GV =
+  new GlobalVariable(*M, StrConstant->getType(), true,
+ GlobalValue::ExternalLinkage, StrConstant, "foo");
+  GV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+  GV->setAlignment(Align(1));
 
   GV->setSection(".debug_str");
 
Index: llvm/unittests/ExecutionEngine/Orc/LegacyRTDyldObjectLinkingLayerTest.cpp
===
--- llvm/unittests/ExecutionEngine/Orc/LegacyRTDyldObjectLinkingLayerTest.cpp
+++ llvm/unittests/ExecutionEngine/Orc/LegacyRTDyldObjectLinkingLayerTest.cpp
@@ -77,10 +77,12 @@
   LLVMContext Context;
   auto M = std::make_unique("", Context);
   M->setTargetTriple("x86_64-unknown-linux-gnu");
-  Type *Int32Ty = IntegerType::get(Context, 32);
-  GlobalVariable *GV =
-new GlobalVariable(*M, Int32Ty, false, GlobalValue::ExternalLinkage,
- ConstantInt::get(Int32Ty, 42), "foo");
+  Constant *StrConstant = ConstantDataArray::getString(Context, "forty-two");
+  auto *GV =
+  new GlobalVariable(*M, StrConstant->getType(), true,
+ GlobalValue::ExternalLinkage, StrConstant, "foo");
+  GV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+  GV->setAlignment(Align(1));
 
   GV->setSection(".debug_str");
 
Index: llvm/test/CodeGen/X86/explicit-section-mergeable.ll
===
--- /dev/null
+++ llvm/test/CodeGen/X86/explicit-section-mergeable.ll
@@ -0,0 +1,296 @@
+; RUN: llc < %s -mtriple=x86_64 -unique-section-names=0 -data-sections 2>&1 \
+; RUN: | FileCheck %s
+
+;; Several sections are created via inline assembly. We add checks
+;; for these lines as we want to use --implicit-check-not to reduce the
+;; number of checks in this file.
+; CHECK: .section .asm_mergeable1,"aMS",@progbits,2
+; CHECK-NEXT: .section .asm_nonmergeable1,"a",@progbits
+; CHECK-NEXT: .section .asm_mergeable2,"aMS",@progbits,2
+; CHECK-NEXT: .section .asm_nonmergeable2,"a",@progbits
+
+;; Test implicit section assignment for symbols
+; CHECK: .section .data,"aw",@progbits,unique,1
+; CHECK: uniquified:
+
+;; Create a uniquified symbol (as -unique-section-names=0) to test the uniqueID
+;; interaction with mergeable symbols.
+@uniquified = global i32 1
+
+;; Test implicit section assignment for symbols to ensure that the symbols
+;; have the expected properties.
+; CHECK: .section .rodata,"a",@progbits,unique,2
+; CHECK: implicit_nonmergeable:
+; CHECK: .section .rodata.cst4,"aM",@progbits,4
+; CHECK: implicit_rodata_cst4:
+; CHECK: .section .rodata.cst8,"aM",@progbits,8
+; CHECK: implicit_rodata_cst8:
+; CHECK: .section .rodata.str4.4,"aMS",@progbits,4
+; CHECK: implicit_rodata_str4_4:
+
+@implicit_nonmergeable  =  constant [2 x i16] [i16 1, i16 1]
+@implicit_rodata_cst4   = unnamed_addr constant [2 x i16] [i16 1, i16 1]
+@implicit_rodata_cst8   = unnamed_addr constant [2 x i32] [i32 1, i32 1]
+@implicit_rodata_str4_4 = unnamed_addr constant [2 x i32] [i32 1, i32 0]
+
+;; Basic checks that mergeable globals are placed into multiple distinct
+;; sections with the same name and a compatible entry size.
+
+; CHECK: .section .explicit_basic,"aM",@progbits,4,unique,3
+; CHECK: explicit_basic_1:
+; CHECK: explicit_basic_2:
+
+;; Assign a mergeable global to a non-existing section.
+@explicit_basic_1 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit_basic"
+;; As

[PATCH] D133266: [MinGW] Reject explicit non-default visibility applied to dllexport/dllimport declaration

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

This approach doesn't account for the implementation of 
-fvisibility-global-new-delete-hidden. The following case now fails after this 
change:

> type new_del.cpp

namespace std {

  typedef __typeof__(sizeof(int)) size_t;
  struct nothrow_t {};
  struct align_val_t {};

}
__declspec(dllexport) void operator delete(void* ptr) throw() {return;}

> clang.exe --target=x86_64-pc-windows-gnu new_del.cpp 
> -fvisibility-global-new-delete-hidden -c

new_del.cpp:8:28: error: non-default visibility cannot be applied to 
'dllexport' declaration
__declspec(dllexport) void operator delete(void* ptr) throw() {return;}

  ^

1 error generated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133266/new/

https://reviews.llvm.org/D133266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133266: [MinGW] Reject explicit non-default visibility applied to dllexport/dllimport declaration

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

In D133266#3775832 , @MaskRay wrote:

> In D133266#3775384 , @MaskRay wrote:
>
>> In D133266#3775189 , @bd1976llvm 
>> wrote:
>>
>>> 
>>
>> From a glance, `-fvisibility-global-new-delete-hidden` should make the 
>> visibility implicit (so won't trigger this error) but don't seem to do so? 
>> I'll investigate later.
>
> `-fvisibility-global-new-delete-hidden` is implemented by adding 
> `VisibilityAttr` to declarations.
> I think  `-fvisibility-global-new-delete-hidden` triggering the error is 
> fine. The alternative, adding a rule that `__declspec(dllexport) void 
> operator delete` does not get hidden visibility, seems ad-hoc to me.

I'm not sure why this visibility option 
(`-fvisibility-global-new-delete-hidden`) is implemented differently to the 
others (e.g. `-fvisibility=hidden`)? `__declspec(dllexport) void operator 
delete` does not get hidden visibility, might be difficult to implement; but 
OTOH, the dllstorageclass overrides the visibility set by a visibility option 
for the other visibility options (e.g. -fvisibility=hidden) and it would be 
nice to have consistent behaviour for all the visibility options. Would be 
great to get other peoples opinions on whether an error here would be a problem?

> I think the only needed change is to allow dllexport protected, but then we 
> probably need two diagnostics. Perhaps `diag::err_hidden_visiblity_dllexport` 
> and `diag::err_non_default_visibility_dllimport`?

SGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133266/new/

https://reviews.llvm.org/D133266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133266: [MinGW] Reject explicit hidden visibility applied to dllexport and hidden/protected applied to dllimport

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

In D133266#3776164 , @MaskRay wrote:

> In D133266#3776051 , @bd1976llvm 
> wrote:
>
>> In D133266#3775832 , @MaskRay 
>> wrote:
>>
>>> In D133266#3775384 , @MaskRay 
>>> wrote:
>>>
 In D133266#3775189 , @bd1976llvm 
 wrote:

> 

 From a glance, `-fvisibility-global-new-delete-hidden` should make the 
 visibility implicit (so won't trigger this error) but don't seem to do so? 
 I'll investigate later.
>>>
>>> `-fvisibility-global-new-delete-hidden` is implemented by adding 
>>> `VisibilityAttr` to declarations.
>>> I think  `-fvisibility-global-new-delete-hidden` triggering the error is 
>>> fine. The alternative, adding a rule that `__declspec(dllexport) void 
>>> operator delete` does not get hidden visibility, seems ad-hoc to me.
>>
>> I'm not sure why this visibility option 
>> (`-fvisibility-global-new-delete-hidden`) is implemented differently to the 
>> others (e.g. `-fvisibility=hidden`)? `__declspec(dllexport) void operator 
>> delete` does not get hidden visibility, might be difficult to implement; but 
>> OTOH, the dllstorageclass overrides the visibility set by a visibility 
>> option for the other visibility options (e.g. -fvisibility=hidden) and it 
>> would be nice to have consistent behaviour for all the visibility options. 
>> Would be great to get other peoples opinions on whether an error here would 
>> be a problem?
>
> First, does COFF use `-fvisibility-global-new-delete-hidden`? The impl of 
> `-fvisibility-global-new-delete-hidden` is very different from -fvisibility. 
> -fvisibility changed visibility is considered `!isVisibilityExplicit` while  
> `-fvisibility-global-new-delete-hidden`'s is `isVisibilityExplicit`.
>
> dllspec and visibility are extensions for different object file formats and 
> here we are mixing them together.
> Personally I don't favor defining too many "let A override B" designs. Adding 
> `if (GV->hasDLLExportStorageClass() || GV->hasDLLImportStorageClass())` is 
> mostly only due to compatibility consideration...

Sorry for the slow reply. -fvisibility-global-new-delete-hidden was implemented 
here: https://reviews.llvm.org/D53787, it is Clang only (it is not in GCC) and, 
as you pointed out, the changed visibility is deliberately 
`isVisibilityExplicit`. We are happy to accept the change in behaviour as we 
don't think it will impact customers.

>>> I think the only needed change is to allow dllexport protected, but then we 
>>> probably need two diagnostics. Perhaps 
>>> `diag::err_hidden_visiblity_dllexport` and 
>>> `diag::err_non_default_visibility_dllimport`?
>>
>> SGTM!

LGTM. Thanks for your patience.




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1116
+if (GV->hasDLLExportStorageClass()) {
+  // Reject explicit hidden visibility on dllexport.
+  if (LV.getVisibility() == HiddenVisibility)

Opinion: I don't think these comments are needed as the code is clear. I think 
it would be better to have a single comment at the top of this block e.g. 
"Reject incompatible dlllstorageclass and visibility annotations" or just 
remove them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133266/new/

https://reviews.llvm.org/D133266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

Sorry for the delay in updating this.

I hoped to post an updated patch today for this but I realized that I first 
need to investigate whether section relative references might interact badly 
with multiple sections with the same name.. I will be able to update the patch 
tomorrow.

Below is the code comment from the new patch explaining the new approach, 
please take a look and see if you have any questions/comments:

  // If two globals with differing sizes end up in the same mergeable
  // section that section can be assigned an incorrect entry size. Normally,
  // the assembler avoids this by putting incompatible globals into
  // differently named sections. However, globals can be explicitly assigned
  // to a section by specifying the section name. In this case, if unique
  // section names are available (-unique-section-names in LLVM) then we
  // bin compatible globals into different mergeable sections with the same 
name.
  // Otherwise, if incompatible globals have been explicitly assigned to 
section by a
  // fine-grained/per-symbol mechanism (e.g. via  
_attribute_((section(“myname” then
  // we issue an error and the user can then change the section assignment. If 
the
  // section assignment is not via a fine-grained means (e.g. via pragma clang 
section)
  // then we simply do not make the resulting section mergeable as there is 
nothing
  // that the user can easily change to fix the resulting problem.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68101/new/

https://reviews.llvm.org/D68101



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

In D68101#1802280 , @rjmccall wrote:

> The solution described in that comment is not acceptable.  We are not going 
> to tell users that they cannot assign symbols explicitly to sections because 
> LLVM is unable to promise not to miscompile if they do.  It is LLVM's 
> responsibility to correctly compile valid code; enabling mergeability for a 
> section containing `unnamed_addr` symbols is an optimization, and if it is 
> not safe, it needs to be disabled until we can figure out a way to make it 
> safe.


Thanks for the feedback. Apologies that you had to repeat yourself.

> I laid out a series of three options before:
> 
> - Emit different object-file sections for different mergeability settings.
> - Only mark an object-file section as mergeable if all the symbols in it 
> would have the same mergeability settings.
> - Stop implicitly using mergeability for "ordinary" sections (i.e. sections 
> other than the string section).
> 
>   Did you investigate these?

I looked into these today. I think we can do the first of these. I have put a 
WIP patch up here: https://reviews.llvm.org/D72194. Could you comment on the 
approach taken?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68101/new/

https://reviews.llvm.org/D68101



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

Thanks for the feedback. I'm sorry that I am being slow on this... v.busy at 
the moment. Will try to finish the patch in the next few days.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68101/new/

https://reviews.llvm.org/D68101



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

In D68101#1806475 , @nickdesaulniers 
wrote:

> In D68101#1806213 , @rjmccall wrote:
>
> > In D68101#1806135 , 
> > @nickdesaulniers wrote:
> >
> > > In D68101#1802280 , @rjmccall 
> > > wrote:
> > >
> > > > I laid out a series of three options before:
> > > >
> > > > - Emit different object-file sections for different mergeability 
> > > > settings.
> > > > - Only mark an object-file section as mergeable if all the symbols in 
> > > > it would have the same mergeability settings.
> > > > - Stop implicitly using mergeability for "ordinary" sections (i.e. 
> > > > sections other than the string section).
> > >
> > >
> > > Of the above, I really think #2 is the only complete solution.
> >
> >
> > Can you explain why you think #1 is not "complete"?  All three seem to 
> > establish correctness; I can see how giving up on the optimization (#3) is 
> > not a great solution, but #1 doesn't have that problem and actually 
> > preserves it in more places.  To be clear, this is just taking advantage of 
> > the ability to have multiple sections with the same name in an ELF object 
> > file; they will still be assembled into a single section in the linked 
> > image.
> >
> > My understanding is that changing the flags on an MCSection retroactively 
> > is pretty counter to the architecture, so I'm not sure how we'd actually 
> > achieve #2.
>
>
> Ah, ok, I reread  https://reviews.llvm.org/D72194 and see that it's creating 
> non-contiguous sections (option 1), with unique entity sizes.  That should be 
> fine. Dissenting opinion retracted.  We should prefer 
> https://reviews.llvm.org/D72194 with the commit message updated.


I have updated the commit message and patch on https://reviews.llvm.org/D72194.

I am abandoning this in favor of https://reviews.llvm.org/D72194.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68101/new/

https://reviews.llvm.org/D68101



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90299: [windows-itanium] handle dllimport/export code paths separately and share with PS4

[ben via Phabricator via cfe-commits]

bd1976llvm marked 2 inline comments as done.
bd1976llvm added a comment.

ping :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90299/new/

https://reviews.llvm.org/D90299

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133266: [MinGW] Reject explicit hidden visibility applied to dllexport and hidden/protected applied to dllimport

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

Whilst reviewing the impact of these dllimport/export restrictions I noticed 
that LLVM IR does not reject dllimport/export on local linkage globals. I have 
put up a PR for adding that restriction: D134784 
 (https://reviews.llvm.org/D134784).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133266/new/

https://reviews.llvm.org/D133266

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138463: [windows-itanium] Propagate DLL storage class to Initialisation Guard Variables

[ben via Phabricator via cfe-commits]

bd1976llvm updated this revision to Diff 477287.
bd1976llvm edited the summary of this revision.
bd1976llvm added a comment.

Fix for test failure on *nix.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138463/new/

https://reviews.llvm.org/D138463

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/windows-itanium-init-guard.cpp


Index: clang/test/CodeGenCXX/windows-itanium-init-guard.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/windows-itanium-init-guard.cpp
@@ -0,0 +1,32 @@
+// Initialisation Guard Variables should take their DLL storage class from
+// the guarded variable. Otherwise, there will be a link error if the compiler
+// inlines a reference to the guard variable into another module but that
+// guard variable is not exported from the defining module.
+
+// RUN: %clang_cc1 -emit-llvm -triple i686-windows-itanium -fdeclspec %s -O1 
-disable-llvm-passes -o - -DAPI= | FileCheck %s --check-prefixes=NONE
+// RUN: %clang_cc1 -emit-llvm -triple i686-windows-itanium -fdeclspec %s -O1 
-disable-llvm-passes -o - -DAPI="__declspec(dllexport)" | FileCheck %s 
--check-prefixes=EXPORT
+// RUN: %clang_cc1 -emit-llvm -triple i686-windows-itanium -fdeclspec %s -O1 
-disable-llvm-passes -o - -DAPI="__declspec(dllimport)" | FileCheck %s 
--check-prefixes=IMPORT
+
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-scei-ps4 -fdeclspec %s -O1 
-disable-llvm-passes -o - -DAPI= | FileCheck %s --check-prefixes=NONE
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-scei-ps4 -fdeclspec %s -O1 
-disable-llvm-passes -o - -DAPI="__declspec(dllexport)" | FileCheck %s 
--check-prefixes=EXPORT
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-scei-ps4 -fdeclspec %s -O1 
-disable-llvm-passes -o - -DAPI="__declspec(dllimport)" | FileCheck %s 
--check-prefixes=IMPORT
+
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-scei-ps5 -fdeclspec %s -O1 
-disable-llvm-passes -o - -DAPI= | FileCheck %s --check-prefixes=NONE
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-scei-ps5 -fdeclspec %s -O1 
-disable-llvm-passes -o - -DAPI="__declspec(dllexport)" | FileCheck %s 
--check-prefixes=EXPORT
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-scei-ps5 -fdeclspec %s -O1 
-disable-llvm-passes -o - -DAPI="__declspec(dllimport)" | FileCheck %s 
--check-prefixes=IMPORT
+
+//NONE: @_ZZN3foo3GetEvE9Singleton = linkonce_odr {{(dso_local )?}}global
+//NONE: @_ZGVZN3foo3GetEvE9Singleton = linkonce_odr {{(dso_local )?}}global
+
+//EXPORT: @_ZZN3foo3GetEvE9Singleton = weak_odr {{(dso_local )?}}dllexport 
global
+//EXPORT: @_ZGVZN3foo3GetEvE9Singleton = weak_odr {{(dso_local )?}}dllexport 
global
+
+//IMPORT: @_ZZN3foo3GetEvE9Singleton = available_externally dllimport global
+//IMPORT: @_ZGVZN3foo3GetEvE9Singleton = available_externally dllimport global
+
+struct API foo {
+  foo() {}
+  static void Get() { static foo Singleton; }
+};
+
+void f() { foo::Get(); }
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -2384,13 +2384,15 @@
 }
 
 // Create the guard variable with a zero-initializer.
-// Just absorb linkage and visibility from the guarded variable.
+// Just absorb linkage, visibility and dll storage class  from the guarded
+// variable.
 guard = new llvm::GlobalVariable(CGM.getModule(), guardTy,
  false, var->getLinkage(),
  llvm::ConstantInt::get(guardTy, 0),
  guardName.str());
 guard->setDSOLocal(var->isDSOLocal());
 guard->setVisibility(var->getVisibility());
+guard->setDLLStorageClass(var->getDLLStorageClass());
 // If the variable is thread-local, so is its guard variable.
 guard->setThreadLocalMode(var->getThreadLocalMode());
 guard->setAlignment(guardAlignment.getAsAlign());


Index: clang/test/CodeGenCXX/windows-itanium-init-guard.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/windows-itanium-init-guard.cpp
@@ -0,0 +1,32 @@
+// Initialisation Guard Variables should take their DLL storage class from
+// the guarded variable. Otherwise, there will be a link error if the compiler
+// inlines a reference to the guard variable into another module but that
+// guard variable is not exported from the defining module.
+
+// RUN: %clang_cc1 -emit-llvm -triple i686-windows-itanium -fdeclspec %s -O1 -disable-llvm-passes -o - -DAPI= | FileCheck %s --check-prefixes=NONE
+// RUN: %clang_cc1 -emit-llvm -triple i686-windows-itanium -fdeclspec %s -O1 -disable-llvm-passes -o - -DAPI="__declspec(dllexport)" | FileCheck %s --check-prefixes=EXPORT
+// RUN: %clang_cc1 -emit-llvm -triple i686-windows-itanium -fdeclspec %s -O1 -disable-llvm-passes -o - -DAPI="__declspec(dllimport)" | FileCheck %s --check-prefixes=IMPORT
+
+// RUN: %

[PATCH] D138463: [windows-itanium] Propagate DLL storage class to Initialisation Guard Variables

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

In D138463#3944124 , @compnerd wrote:

> The change itself seems fine, but the CI failure seems related to this change.

Thanks. There's been renewed interest in using DLLs with C++ interfaces so we 
should, hopefully, be able to put some effort into improving the support.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138463/new/

https://reviews.llvm.org/D138463

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

In D68101#1689622 , @SjoerdMeijer 
wrote:

> Just to make sure I haven't missed anything, I would like to take this patch 
> and run some numbers, for which I need a little bit of time.


Hi @SjoerdMeijer. Did you have a chance to run the numbers? I am keen to get a 
fix in for at least the SHF_MERGE issue as we have customers who have hit this 
case. If you haven't had time I will work on a lower risk patch (lower risk of 
performance impact). Thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68101/new/

https://reviews.llvm.org/D68101



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

In D68101#1766151 , @nickdesaulniers 
wrote:

> In D68101#1765665 , @rjmccall wrote:
>
> > Given all that, this patch seems far too aggressive.  While mergeable 
> > sections can be useful for optimizing arbitrary code that might not use a 
> > section, they are also extremely useful for optimizing the sorts of global 
> > tables that programmers frequently use explicit sections for.  It seems to 
> > me that the right fix is to find the place that ensures that we don't put 
> > mergeable and non-mergeable objects in the same section unit (or at least 
> > conservatively makes the section unit non-mergeable) and fix it to consider 
> > entry size as well.  That should be straightforward unless that place 
> > doesn't exist, in which case we have very serious problems.
>
>
> Disagree (but I spent all day thinking about this).  Going back to 
> https://bugs.llvm.org/show_bug.cgi?id=31828, I think I solved the problem 
> there incorrectly; we should be not marking the explicit section merge-able.


Thanks for the comments!

I am convinced that the current patch is too pessimistic, size optimizations 
can be functional requirements in memory constrained areas.

There are two frontend attributes involved:

pragma clang section ... - 
https://clang.llvm.org/docs/LanguageExtensions.html#specifying-section-names-for-global-objects-pragma-clang-section
_attribute_ ((section ("section-name"))) - 
https://clang.llvm.org/docs/AttributeReference.html#section-declspec-allocate

At the IR level you can query for explicit sections assignment with code like:

  static bool inExplicitSection(const GlobalObject *GO, SectionKind Kind) {
    if (GO->hasSection())
      return true;
  
    if (auto *GVar = dyn_cast(GO)) {
      auto Attrs = GVar->getAttributes();
      if ((Attrs.hasAttribute("bss-section") && Kind.isBSS()) ||
          (Attrs.hasAttribute("data-section") && Kind.isData()) ||
          (Attrs.hasAttribute("rodata-section") && Kind.isReadOnly())) {
        return true;
      }
    }
  
    if (auto *F = dyn_cast(GO)) {
      if (F->hasFnAttribute("implicit-section-name"))
        return true;
    }
  
    return false;
  }

The section of a global is used for _attribute_ ((section ("section-name"))) 
but it is also set in LLVM e.g. by optimization passes.
Attributes "bss-section", "data-section", "rodata-section" and 
"implicit-section-name" are only used for "clang section" pragmas.

As I commented upthread I would like input from the authors of the "clang 
section" pragma to understand whether the semantics of that pragma would allow 
for generating multiple output sections (with the same name). Assuming that 
this pragma implies a single output section I have an new proposal for a less 
pessimistic fix:

1. Sections generated for "clang section" pragmas are never made mergeable.
2. It is an error if incompatible globals are assigned to a section with the 
same name.

This works as the user can easily fix any "_attribute_ ((section 
("section-name")))" in their source code and internal uses in LLVM code can 
also make sure that incompatible globals are assigned to differently named 
sections. Thoughts? I'll update the patch shortly unless there are objections.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68101/new/

https://reviews.llvm.org/D68101



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

In D68101#1766359 , @rjmccall wrote:

> I can't speak for the pragma authors, but I strongly doubt the pragma is 
> intended to force all affected globals to go into a single section unit, 
> since the division into section units is purely an object-file representation 
> issue.


I'm thinking of embedded platforms where there is no or only very primitive 
linkers. There is also the advantage of producing similar output to GCC. 
Creating multiple output sections is not without risk - for example, it means 
that symbols can be re-ordered (relative to their original order in source 
files) which could cause a change in behaviour.

Perhaps I am being too cautious - we have had bugs and reviews open for long 
enough for anyone interested to comment. The way forward now, I think, is to 
make a reasonable fix and see what breaks.

> Looks to me like `MCContext::getELFSection` should be including the entry 
> size and flags as part of the uniquing key.

Agreed. I will update the patch to do that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68101/new/

https://reviews.llvm.org/D68101



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits