r341373 - Fix the -print-multi-directory flag to print the selected multilib.

[Christian Bruel via cfe-commits]

Author: chrib
Date: Tue Sep  4 08:22:13 2018
New Revision: 341373

URL: http://llvm.org/viewvc/llvm-project?rev=341373&view=rev
Log:
Fix the -print-multi-directory flag to print the selected multilib.

Summary: Fix -print-multi-directory to print the selected multilib

Reviewers: jroelofs

Reviewed By: jroelofs

Subscribers: srhines, cfe-commits

Differential Revision: https://reviews.llvm.org/D51354

Added:
cfe/trunk/test/Driver/print-multi-directory.c
Modified:
cfe/trunk/include/clang/Driver/ToolChain.h
cfe/trunk/lib/Driver/Driver.cpp
cfe/trunk/lib/Driver/ToolChains/Linux.cpp

Modified: cfe/trunk/include/clang/Driver/ToolChain.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/ToolChain.h?rev=341373&r1=341372&r2=341373&view=diff
==
--- cfe/trunk/include/clang/Driver/ToolChain.h (original)
+++ cfe/trunk/include/clang/Driver/ToolChain.h Tue Sep  4 08:22:13 2018
@@ -149,6 +149,7 @@ private:
 
 protected:
   MultilibSet Multilibs;
+  Multilib SelectedMultilib;
 
   ToolChain(const Driver &D, const llvm::Triple &T,
 const llvm::opt::ArgList &Args);
@@ -227,6 +228,8 @@ public:
 
   const MultilibSet &getMultilibs() const { return Multilibs; }
 
+  const Multilib &getMultilib() const { return SelectedMultilib; }
+
   const SanitizerArgs& getSanitizerArgs() const;
 
   const XRayArgs& getXRayArgs() const;

Modified: cfe/trunk/lib/Driver/Driver.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=341373&r1=341372&r2=341373&view=diff
==
--- cfe/trunk/lib/Driver/Driver.cpp (original)
+++ cfe/trunk/lib/Driver/Driver.cpp Tue Sep  4 08:22:13 2018
@@ -1661,14 +1661,13 @@ bool Driver::HandleImmediateArgs(const C
   }
 
   if (C.getArgs().hasArg(options::OPT_print_multi_directory)) {
-for (const Multilib &Multilib : TC.getMultilibs()) {
-  if (Multilib.gccSuffix().empty())
-llvm::outs() << ".\n";
-  else {
-StringRef Suffix(Multilib.gccSuffix());
-assert(Suffix.front() == '/');
-llvm::outs() << Suffix.substr(1) << "\n";
-  }
+const Multilib &Multilib = TC.getMultilib();
+if (Multilib.gccSuffix().empty())
+  llvm::outs() << ".\n";
+else {
+  StringRef Suffix(Multilib.gccSuffix());
+  assert(Suffix.front() == '/');
+  llvm::outs() << Suffix.substr(1) << "\n";
 }
 return false;
   }

Modified: cfe/trunk/lib/Driver/ToolChains/Linux.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Linux.cpp?rev=341373&r1=341372&r2=341373&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Linux.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Linux.cpp Tue Sep  4 08:22:13 2018
@@ -210,6 +210,7 @@ Linux::Linux(const Driver &D, const llvm
 : Generic_ELF(D, Triple, Args) {
   GCCInstallation.init(Triple, Args);
   Multilibs = GCCInstallation.getMultilibs();
+  SelectedMultilib = GCCInstallation.getMultilib();
   llvm::Triple::ArchType Arch = Triple.getArch();
   std::string SysRoot = computeSysRoot();
 
@@ -299,16 +300,14 @@ Linux::Linux(const Driver &D, const llvm
   if (GCCInstallation.isValid()) {
 const llvm::Triple &GCCTriple = GCCInstallation.getTriple();
 const std::string &LibPath = GCCInstallation.getParentLibPath();
-const Multilib &Multilib = GCCInstallation.getMultilib();
-const MultilibSet &Multilibs = GCCInstallation.getMultilibs();
 
 // Add toolchain / multilib specific file paths.
-addMultilibsFilePaths(D, Multilibs, Multilib,
+addMultilibsFilePaths(D, Multilibs, SelectedMultilib,
   GCCInstallation.getInstallPath(), Paths);
 
 // Sourcery CodeBench MIPS toolchain holds some libraries under
 // a biarch-like suffix of the GCC installation.
-addPathIfExists(D, GCCInstallation.getInstallPath() + Multilib.gccSuffix(),
+addPathIfExists(D, GCCInstallation.getInstallPath() + 
SelectedMultilib.gccSuffix(),
 Paths);
 
 // GCC cross compiling toolchains will install target libraries which ship
@@ -330,7 +329,7 @@ Linux::Linux(const Driver &D, const llvm
 // Note that this matches the GCC behavior. See the below comment for where
 // Clang diverges from GCC's behavior.
 addPathIfExists(D, LibPath + "/../" + GCCTriple.str() + "/lib/../" +
-   OSLibDir + Multilib.osSuffix(),
+   OSLibDir + SelectedMultilib.osSuffix(),
 Paths);
 
 // If the GCC installation we found is inside of the sysroot, we want to

Added: cfe/trunk/test/Driver/print-multi-directory.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/print-multi-directory.c?rev=341373&view=auto
==
--- cfe/trun

Re: r341373 - Fix the -print-multi-directory flag to print the selected multilib.

[Christian BRUEL via cfe-commits]

Oh, yes,  sure

Best Regards

Christian


On 09/05/2018 12:05 AM, Richard Smith wrote:
This is breaking buildbots:

http://lab.llvm.org:8011/builders/clang-x86_64-linux-selfhost-modules/builds/19509

Can you take a look? Thanks!

On Tue, 4 Sep 2018 at 08:36, Christian Bruel via cfe-commits 
mailto:cfe-commits@lists.llvm.org>> wrote:
Author: chrib
Date: Tue Sep  4 08:22:13 2018
New Revision: 341373

URL: http://llvm.org/viewvc/llvm-project?rev=341373&view=rev
Log:
Fix the -print-multi-directory flag to print the selected multilib.

Summary: Fix -print-multi-directory to print the selected multilib

Reviewers: jroelofs

Reviewed By: jroelofs

Subscribers: srhines, cfe-commits

Differential Revision: https://reviews.llvm.org/D51354

Added:
cfe/trunk/test/Driver/print-multi-directory.c
Modified:
cfe/trunk/include/clang/Driver/ToolChain.h
cfe/trunk/lib/Driver/Driver.cpp
cfe/trunk/lib/Driver/ToolChains/Linux.cpp

Modified: cfe/trunk/include/clang/Driver/ToolChain.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/ToolChain.h?rev=341373&r1=341372&r2=341373&view=diff
==
--- cfe/trunk/include/clang/Driver/ToolChain.h (original)
+++ cfe/trunk/include/clang/Driver/ToolChain.h Tue Sep  4 08:22:13 2018
@@ -149,6 +149,7 @@ private:

 protected:
   MultilibSet Multilibs;
+  Multilib SelectedMultilib;

   ToolChain(const Driver &D, const llvm::Triple &T,
 const llvm::opt::ArgList &Args);
@@ -227,6 +228,8 @@ public:

   const MultilibSet &getMultilibs() const { return Multilibs; }

+  const Multilib &getMultilib() const { return SelectedMultilib; }
+
   const SanitizerArgs& getSanitizerArgs() const;

   const XRayArgs& getXRayArgs() const;

Modified: cfe/trunk/lib/Driver/Driver.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=341373&r1=341372&r2=341373&view=diff
==
--- cfe/trunk/lib/Driver/Driver.cpp (original)
+++ cfe/trunk/lib/Driver/Driver.cpp Tue Sep  4 08:22:13 2018
@@ -1661,14 +1661,13 @@ bool Driver::HandleImmediateArgs(const C
   }

   if (C.getArgs().hasArg(options::OPT_print_multi_directory)) {
-for (const Multilib &Multilib : TC.getMultilibs()) {
-  if (Multilib.gccSuffix().empty())
-llvm::outs() << ".\n";
-  else {
-StringRef Suffix(Multilib.gccSuffix());
-assert(Suffix.front() == '/');
-llvm::outs() << Suffix.substr(1) << "\n";
-  }
+const Multilib &Multilib = TC.getMultilib();
+if (Multilib.gccSuffix().empty())
+  llvm::outs() << ".\n";
+else {
+  StringRef Suffix(Multilib.gccSuffix());
+  assert(Suffix.front() == '/');
+  llvm::outs() << Suffix.substr(1) << "\n";
 }
 return false;
   }

Modified: cfe/trunk/lib/Driver/ToolChains/Linux.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Linux.cpp?rev=341373&r1=341372&r2=341373&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Linux.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Linux.cpp Tue Sep  4 08:22:13 2018
@@ -210,6 +210,7 @@ Linux::Linux(const Driver &D, const llvm
 : Generic_ELF(D, Triple, Args) {
   GCCInstallation.init(Triple, Args);
   Multilibs = GCCInstallation.getMultilibs();
+  SelectedMultilib = GCCInstallation.getMultilib();
   llvm::Triple::ArchType Arch = Triple.getArch();
   std::string SysRoot = computeSysRoot();

@@ -299,16 +300,14 @@ Linux::Linux(const Driver &D, const llvm
   if (GCCInstallation.isValid()) {
 const llvm::Triple &GCCTriple = GCCInstallation.getTriple();
 const std::string &LibPath = GCCInstallation.getParentLibPath();
-const Multilib &Multilib = GCCInstallation.getMultilib();
-const MultilibSet &Multilibs = GCCInstallation.getMultilibs();

 // Add toolchain / multilib specific file paths.
-addMultilibsFilePaths(D, Multilibs, Multilib,
+addMultilibsFilePaths(D, Multilibs, SelectedMultilib,
   GCCInstallation.getInstallPath(), Paths);

 // Sourcery CodeBench MIPS toolchain holds some libraries under
 // a biarch-like suffix of the GCC installation.
-addPathIfExists(D, GCCInstallation.getInstallPath() + Multilib.gccSuffix(),
+addPathIfExists(D, GCCInstallation.getInstallPath() + 
SelectedMultilib.gccSuffix(),
 Paths);

 // GCC cross compiling toolchains will install target libraries which ship
@@ -330,7 +329,7 @@ Linux::Linux(const Driver &D, const llvm
 // Note that this matches the GCC behavior. See the below comment for where
 // Clang diverges from GCC's behavior.
 addPathIfExi

Re: [PATCH] D51354: Fix the -print-multi-directory flag to print the selected multilib.

[Christian BRUEL via cfe-commits]

Hello all,

Is there a way to see how the patch looked like after my commit and 
before the revert from the git mirror ?

I cannot reproduce the test failure from the bots in my local build, 
(which is based on the the git mirror). but I noticed that a similar 
behavior however occurs when the test patch is applied twice. (I have 
exactly the same error if I duplicate the lines in print-multi-directory.c)

I'm curious to see if there was a difference between the review D51354 
patch and the actual svn commit with arcanist that I might not have used 
correctly.

That was actually my first commit. What I did was from a clean 
subversion update

arc patch D51354
arc commit --revision D51354

I'm not sure what could be wrong, any idea ?

thanks

Christian

On 09/05/2018 12:11 AM, Tim Shen via Phabricator wrote:
> timshen added a comment.
>
>> The test fails on my system like so:
> I also observed the same failure.
>
> Bots also fail: 
> http://lab.llvm.org:8011/builders/clang-x86_64-linux-selfhost-modules/builds/19509/steps/check-all/logs/FAIL%3A%20Clang%3A%3Aprint-multi-directory.c
>
> I'm going to revert this patch.
>
>
> Repository:
>rC Clang
>
> https://reviews.llvm.org/D51354
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: D51354: Fix the -print-multi-directory flag to print the selected multilib.

[Christian BRUEL via cfe-commits]

Understood, The test worked for me because I have both 64 bit and 32 bit libs 
installed on my default sysroot. Otherwise might fail indeed.

Will amend the test with an explicit sysroot, 


> -Original Message-
> From: Tim Shen via Phabricator [mailto:revi...@reviews.llvm.org]
> Sent: Wednesday, September 05, 2018 12:12 AM
> To: Christian BRUEL ; jroel...@jroelofs.com
> Cc: tims...@google.com; srhi...@google.com; cfe-commits@lists.llvm.org
> Subject: [PATCH] D51354: Fix the -print-multi-directory flag to print the
> selected multilib.
> 
> timshen added a comment.
> 
> > The test fails on my system like so:
> 
> I also observed the same failure.
> 
> Bots also fail: http://lab.llvm.org:8011/builders/clang-x86_64-linux-selfhost-
> modules/builds/19509/steps/check-all/logs/FAIL%3A%20Clang%3A%3Aprint-
> multi-directory.c
> 
> I'm going to revert this patch.
> 
> 
> Repository:
>   rC Clang
> 
> https://reviews.llvm.org/D51354
> 
> 

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

r341547 - Fix the -print-multi-directory flag to print the selected multilib.

[Christian Bruel via cfe-commits]

Author: chrib
Date: Thu Sep  6 07:03:44 2018
New Revision: 341547

URL: http://llvm.org/viewvc/llvm-project?rev=341547&view=rev
Log:
Fix the -print-multi-directory flag to print the selected multilib.

Summary: Fix -print-multi-directory to print the selected multilib

Reviewers: jroelofs

Reviewed By: jroelofs

Subscribers: jroelofs, timshen, thakis, srhines, cfe-commits

Differential Revision: https://reviews.llvm.org/D51354

Added:
cfe/trunk/test/Driver/print-multi-directory.c
Modified:
cfe/trunk/include/clang/Driver/ToolChain.h
cfe/trunk/lib/Driver/Driver.cpp
cfe/trunk/lib/Driver/ToolChains/Linux.cpp

Modified: cfe/trunk/include/clang/Driver/ToolChain.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/ToolChain.h?rev=341547&r1=341546&r2=341547&view=diff
==
--- cfe/trunk/include/clang/Driver/ToolChain.h (original)
+++ cfe/trunk/include/clang/Driver/ToolChain.h Thu Sep  6 07:03:44 2018
@@ -149,6 +149,7 @@ private:
 
 protected:
   MultilibSet Multilibs;
+  Multilib SelectedMultilib;
 
   ToolChain(const Driver &D, const llvm::Triple &T,
 const llvm::opt::ArgList &Args);
@@ -227,6 +228,8 @@ public:
 
   const MultilibSet &getMultilibs() const { return Multilibs; }
 
+  const Multilib &getMultilib() const { return SelectedMultilib; }
+
   const SanitizerArgs& getSanitizerArgs() const;
 
   const XRayArgs& getXRayArgs() const;

Modified: cfe/trunk/lib/Driver/Driver.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=341547&r1=341546&r2=341547&view=diff
==
--- cfe/trunk/lib/Driver/Driver.cpp (original)
+++ cfe/trunk/lib/Driver/Driver.cpp Thu Sep  6 07:03:44 2018
@@ -1661,14 +1661,13 @@ bool Driver::HandleImmediateArgs(const C
   }
 
   if (C.getArgs().hasArg(options::OPT_print_multi_directory)) {
-for (const Multilib &Multilib : TC.getMultilibs()) {
-  if (Multilib.gccSuffix().empty())
-llvm::outs() << ".\n";
-  else {
-StringRef Suffix(Multilib.gccSuffix());
-assert(Suffix.front() == '/');
-llvm::outs() << Suffix.substr(1) << "\n";
-  }
+const Multilib &Multilib = TC.getMultilib();
+if (Multilib.gccSuffix().empty())
+  llvm::outs() << ".\n";
+else {
+  StringRef Suffix(Multilib.gccSuffix());
+  assert(Suffix.front() == '/');
+  llvm::outs() << Suffix.substr(1) << "\n";
 }
 return false;
   }

Modified: cfe/trunk/lib/Driver/ToolChains/Linux.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Linux.cpp?rev=341547&r1=341546&r2=341547&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Linux.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Linux.cpp Thu Sep  6 07:03:44 2018
@@ -210,6 +210,7 @@ Linux::Linux(const Driver &D, const llvm
 : Generic_ELF(D, Triple, Args) {
   GCCInstallation.init(Triple, Args);
   Multilibs = GCCInstallation.getMultilibs();
+  SelectedMultilib = GCCInstallation.getMultilib();
   llvm::Triple::ArchType Arch = Triple.getArch();
   std::string SysRoot = computeSysRoot();
 
@@ -299,16 +300,14 @@ Linux::Linux(const Driver &D, const llvm
   if (GCCInstallation.isValid()) {
 const llvm::Triple &GCCTriple = GCCInstallation.getTriple();
 const std::string &LibPath = GCCInstallation.getParentLibPath();
-const Multilib &Multilib = GCCInstallation.getMultilib();
-const MultilibSet &Multilibs = GCCInstallation.getMultilibs();
 
 // Add toolchain / multilib specific file paths.
-addMultilibsFilePaths(D, Multilibs, Multilib,
+addMultilibsFilePaths(D, Multilibs, SelectedMultilib,
   GCCInstallation.getInstallPath(), Paths);
 
 // Sourcery CodeBench MIPS toolchain holds some libraries under
 // a biarch-like suffix of the GCC installation.
-addPathIfExists(D, GCCInstallation.getInstallPath() + Multilib.gccSuffix(),
+addPathIfExists(D, GCCInstallation.getInstallPath() + 
SelectedMultilib.gccSuffix(),
 Paths);
 
 // GCC cross compiling toolchains will install target libraries which ship
@@ -330,7 +329,7 @@ Linux::Linux(const Driver &D, const llvm
 // Note that this matches the GCC behavior. See the below comment for where
 // Clang diverges from GCC's behavior.
 addPathIfExists(D, LibPath + "/../" + GCCTriple.str() + "/lib/../" +
-   OSLibDir + Multilib.osSuffix(),
+   OSLibDir + SelectedMultilib.osSuffix(),
 Paths);
 
 // If the GCC installation we found is inside of the sysroot, we want to

Added: cfe/trunk/test/Driver/print-multi-directory.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/print-multi-directory.c?rev=341547&view=auto

r342031 - Fix Check test to avoid output string mismatch

[Christian Bruel via cfe-commits]

Author: chrib
Date: Wed Sep 12 01:59:17 2018
New Revision: 342031

URL: http://llvm.org/viewvc/llvm-project?rev=342031&view=rev
Log:
Fix Check test to avoid output string mismatch

Differential Revision: http://reviews.llvm.org/D51354


Modified:
cfe/trunk/test/Driver/print-multi-directory.c

Modified: cfe/trunk/test/Driver/print-multi-directory.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/print-multi-directory.c?rev=342031&r1=342030&r2=342031&view=diff
==
--- cfe/trunk/test/Driver/print-multi-directory.c (original)
+++ cfe/trunk/test/Driver/print-multi-directory.c Wed Sep 12 01:59:17 2018
@@ -2,21 +2,19 @@
 // RUN: -target i386-none-linux \
 // RUN: -B%S/Inputs/multilib_64bit_linux_tree/usr \
 // RUN: -print-multi-directory \
-// RUN:   | FileCheck --check-prefix=CHECK-X86-MULTILIBS %s
+// RUN:   | FileCheck --match-full-lines --check-prefix=CHECK-X86-MULTILIBS %s
 
 // CHECK-X86-MULTILIBS:  32
-// CHECK-X86-MULTILIBS-NOT:  x32
-// CHECK-X86-MULTILIBS-NOT:  .
+// CHECK-X86-MULTILIBS-NOT:  {{^.+$}}
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>/dev/null \
 // RUN: -target i386-none-linux -m64 \
 // RUN: -B%S/Inputs/multilib_64bit_linux_tree/usr \
 // RUN: -print-multi-directory \
-// RUN:   | FileCheck --check-prefix=CHECK-X86_64-MULTILIBS %s
+// RUN:   | FileCheck --match-full-lines --check-prefix=CHECK-X86_64-MULTILIBS 
%s
 
 // CHECK-X86_64-MULTILIBS:  .
-// CHECK-X86_64-MULTILIBS-NOT:  x32
-// CHECK-X86_64-MULTILIBS-NOT:  32
+// CHECK-X86_64-MULTILIBS-NOT:  {{^.+$}}
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>/dev/null \
 // RUN: -target arm-linux-androideabi21 \
@@ -24,7 +22,7 @@
 // RUN: -B%S/Inputs/basic_android_ndk_tree \
 // RUN: --sysroot=%S/Inputs/basic_android_ndk_tree/sysroot \
 // RUN: -print-multi-directory \
-// RUN:   | FileCheck  --check-prefix=CHECK-ARM-MULTILIBS %s
+// RUN:   | FileCheck --match-full-lines --check-prefix=CHECK-ARM-MULTILIBS %s
 
 // CHECK-ARM-MULTILIBS:  thumb
-// CHECK-ARM-MULTILIBS-NOT:  .
+// CHECK-ARM-MULTILIBS-NOT:  {{^.+$}}


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

Re: [PATCH] D52533: [test] Use --sysroot instead of -B in print-multi-directory.c

[Christian BRUEL via cfe-commits]

Hi Martin,

Actually I reread the developer policy, 
(https://llvm.org/docs/DeveloperPolicy.html#id13). As I contributed this 
code and take responsibility for it. It seems that I can approve this patch.

So it looks OK for me, sorry for the delay,

Cheers

Christian

On 10/02/2018 07:55 PM, Martin Storsjö via Phabricator wrote:
> mstorsjo added a comment.
>
> Ping @jroelofs or someone else willing to have a look
>
>
> Repository:
>rC Clang
>
> https://reviews.llvm.org/D52533
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D31140: [LLVMbugs] [Bug 18710] Only generate .ARM.exidx and .ARM.extab when needed in EHABI

[Christian Bruel via cfe-commits]

Just to add to our comments bellow,

One experiment I did is to use the nounwind/uwtable combination to 
achieve what we need.

e.g:

nounwind= emit nothing
nounwind uwtable  = emit the .exidx .extab section with the CANTUNWIND

However I gave that up because of tests legacy. e.g

./CodeGen/ARM/ehabi.ll we test :

; (6) .cantunwind directive should be available if the function is 
marked with

; nounwind function attribute.

So many tests would need to be rewritten as
; (6) .cantunwind directive should be available if the function is 
marked with

; nounwind uwtable function attributes

So I'm not sure in which extend we can change attribute semantic if this 
implies adapting the regressions tests ?


But if this is acceptable I'm ready to try this... any other opinion ?

Cheers

Christian

Le 03/21/2017 à 09:11 PM, Jonathan Roelofs a écrit :



On 3/21/17 1:53 PM, Christian Bruel via Phabricator wrote:

chrib added a comment.

In https://reviews.llvm.org/D31140#706411, @jroelofs wrote:


Can you clarify the logic here? It's my understanding that:

`-fno-exceptions` does *not* imply `-fno-unwind-tables`

however:

`-fno-unwind-tables` *does* imply that exceptions cannot be used on 
targets that require the tables to do unwinding.


Yes, (bad things might happen or (std::terminate will be called, or 
destructors not called.)...


But -f[no]-unwind-tables implies the UWTable attribute, not NoUwind 
attribute. To toggle NoUnwind, use -fno-exceptions


And this is getting worse with .canunwind which means DoesNotThrow :)

in my understanding,  the logic is as follow:

Since "An exception cannot propagate through a function with a 
nounwind table. The exception handling runtime environment terminates 
the program if it encounters a nounwind table during exception 
processing." (ARM Information Center)


The "nounwind" LLVM attribute, which means "Function does not throw" 
translates as the EXIDX_CANTUNWIND value in the exception table index 
table which needs to be created for the purpose (for the function)


I think the problem is here, actually. "nounwind" implies "does not 
throw", but "does not throw" really should not imply "nounwind". This 
is something that ought to be clarified in the langref with the 
addition of a "does not throw" attribute. Then the optimizer should be 
fixed to deduce "does not throw" instead of "nounwind", and we can let 
"nounwind" continue to imply .cantunwind.




And of course without exception runtime environment (the test here) 
we don't need this table. So I can see 3 cases:


- nounwind set :Generate .cantunwind directive 
and unwind table
- nounwind set but not EH   Do not generate the .cantunwind directive 
and do not emit the unwind table
- uwtable set Need to generate the unwind 
table (even without EH)


The  disable-arm-cantunwind flag means: without EH support if the 
function does not throw, do dot generate the exception tables and the 
EXIDX_CANTUNWIND value.


I'm not a big fan of this workaround flag. I'd rather see this fixed 
by clarifying/fixing the semantics of the IR.



Jon




https://reviews.llvm.org/D31140







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

Re: [PATCH] D31140: [LLVMbugs] [Bug 18710] Only generate .ARM.exidx and .ARM.extab when needed in EHABI

[Christian Bruel via cfe-commits]

Hi Jon


Le 03/21/2017 à 09:11 PM, Jonathan Roelofs a écrit :



On 3/21/17 1:53 PM, Christian Bruel via Phabricator wrote:

chrib added a comment.

In https://reviews.llvm.org/D31140#706411, @jroelofs wrote:


Can you clarify the logic here? It's my understanding that:

`-fno-exceptions` does *not* imply `-fno-unwind-tables`

however:

`-fno-unwind-tables` *does* imply that exceptions cannot be used on 
targets that require the tables to do unwinding.


Yes, (bad things might happen or (std::terminate will be called, or 
destructors not called.)...


But -f[no]-unwind-tables implies the UWTable attribute, not NoUwind 
attribute. To toggle NoUnwind, use -fno-exceptions


And this is getting worse with .canunwind which means DoesNotThrow :)

in my understanding,  the logic is as follow:

Since "An exception cannot propagate through a function with a 
nounwind table. The exception handling runtime environment terminates 
the program if it encounters a nounwind table during exception 
processing." (ARM Information Center)


The "nounwind" LLVM attribute, which means "Function does not throw" 
translates as the EXIDX_CANTUNWIND value in the exception table index 
table which needs to be created for the purpose (for the function)


I think the problem is here, actually. "nounwind" implies "does not 
throw", but "does not throw" really should not imply "nounwind". This 
is something that ought to be clarified in the langref with the 
addition of a "does not throw" attribute. Then the optimizer should be 
fixed to deduce "does not throw" instead of "nounwind", and we can let 
"nounwind" continue to imply .cantunwind.


Absolutely, renaming the nounwind attribute into a nothrow would make 
things lot clearer! That would fix the semantic (which is already a lot).


This has been already discussed here and there. e.g for reference this 
thread

http://lists.llvm.org/pipermail/llvm-dev/2014-February/070366.html

without any outcome, because the optimizer cannot deduce, without a 
context information from clang, if the environment supports EH or not.


One could imaging a global (LTO) analysis to find any cxa_throw in the 
IRs, but that would not pass the pre-compiled object barrier or dynamic 
dependencies.


So this context information could be well derived either from the 
language (! C) or the fno-exception flag. Unfortunately none of them is 
known to the optimizer.


The proposal was to make this information  of the form of an option 
(like here) or eventually an additional attribute of the form 
nocantunwind (or noeh, any suggestion welcome). So


attributes #0 = { nounwind nocantunwind}

means for the arm streamer dont' emit the .cantunwind directive required 
by the EHABI if the function cant take exceptions






And of course without exception runtime environment (the test here) 
we don't need this table. So I can see 3 cases:


- nounwind set :Generate .cantunwind directive 
and unwind table
- nounwind set but not EH   Do not generate the .cantunwind directive 
and do not emit the unwind table
- uwtable set Need to generate the unwind 
table (even without EH)


The  disable-arm-cantunwind flag means: without EH support if the 
function does not throw, do dot generate the exception tables and the 
EXIDX_CANTUNWIND value.


I'm not a big fan of this workaround flag. I'd rather see this fixed 
by clarifying/fixing the semantics of the IR.




yes we can clarify the semantic of the IR. But that will not be enough 
to fix the problem.


nounwind implies emit .cantunwind which implies exception table
nounwind + NOEH implies don't emit the exception table

and we need this NOEH from clang. Now the question is how?

1) a new arm specific flag
2) a target independant no-exception flag
3) a nocantunwind attribute

But I'm not a big fan of adding a new attribute for a arm specific need...

Best Regards

Christian



Jon




https://reviews.llvm.org/D31140







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