earlephilhower / esp-quick-toolchain

GCC toolchain for esp8266/arduino on MacOS, Linux, ARM64, Raspberry Pi, and Windows
87 stars 24 forks source link

__FUNCTION__, __PRETTY_FUNCTION__ and __func__ not emitted to PROGMEM #9

Closed mikee47 closed 4 years ago

mikee47 commented 4 years ago

Between GCC 8.2.0 and GCC 9.2.1 a change was made which means that strings are no longer emitted to named sections in .cpp code. This means the linker script cannot move them into .irom0.text.

I thought this might be a side-effect of one of the xtensa compiler patches, but this issue is present in the main Linux release for GCC 9.2.1 as well so rules that out. It's not present in GCC 8.2.0 (MinGW).

Try compiling this code as a .cpp file and a .c file and compare the intermediate assembly output:

const char* testFunction() {
    return __FUNCTION__;
}

In the .c version, the output is as expected:

    .section    .rodata.__FUNCTION__$1238,"a"
    .string "testFunction"

But in the .cpp output, we get this:

    .section    .rodata._Z12testFunctionv.str1.1,"aMS",@progbits,1
    .string "testFunction"

The same thing happens with __PRETTY__FUNCTION__ and __func__.

mikee47 commented 4 years ago

I'm not sure this is the appropriate place to open this issue, but perhaps someone can redirect?

earlephilhower commented 4 years ago

Err, not much we can do here if GCC has renamed something under the hood. The GCC mailing list might be the only way to get action on this.

mikee47 commented 4 years ago

OK, thanks. Will have to peg this one up as a 'known issue' for now.

Icing on the cake here, we can't even filter on the 'new' section names because it chucks the strings in with other regular .rodata stuff - "donkey" and "kingpin" are regular const char* here:

.section    .rodata._ZN10Base64Test7executeEv.str1.1,"aMS",@progbits,1
.string "donkey"
.string "kingpin"
.string "virtual void Base64Test::execute()"
earlephilhower commented 4 years ago

Ouch. Like I mentioned in the Sming repo, if you can find the PR in the GCC9 commit logs which caused this, I can try and undo it as part of the patches I apply to GCC (the mforce32, the jump tables inline, etc.) when its built.

earlephilhower commented 4 years ago

This looks like the culprit:

commit c3a961ad27fc21a3af847693820fa9c04dd00940
Author: marxin <marxin@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Thu Nov 1 09:19:31 2018 +0000

    Make __PRETTY_FUNCTION__-like functions mergeable string csts (PR c++/64266).

    2018-11-01  Martin Liska  <mliska@suse.cz>
                Jason Merrill  <jason@redhat.com>

            PR c++/64266
            PR bootstrap/70422
            PR ipa/81277
            * cp-tree.h (DECL_FNAME_P): New macro.
            * decl.c (cp_make_fname_decl): Set DECL_DECLARED_CONSTEXPR_P,
            DECL_VALUE_EXPR, DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P.
            (cp_finish_decl):
            * lambda.c (is_capture_proxy): Use DECL_FNAME_P.
            * pt.c (tsubst_expr): Handle DECL_PRETTY_FUNCTION_P.
    2018-11-01  Martin Liska  <mliska@suse.cz>
                Jason Merrill  <jason@redhat.com>

            PR c++/64266
            PR bootstrap/70422
            PR ipa/81277
            * g++.dg/cpp0x/constexpr-__func__2.C: Make it a compilation
            test.
            * g++.old-deja/g++.ext/pretty4.C: Remove as the run-time
            assumptions are not longer valid.

    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@265711 138bc75d-0d04-0410-961f-82ee72b054a4
mikee47 commented 4 years ago

This patch fixes the problem, can anyone familiar with GCC internals check it?

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index b57ded813b6..8c954d4efcd 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -4495,8 +4495,6 @@ cp_make_fname_decl (location_t loc, tree id, int type_dep)

   if (init)
     {
-      SET_DECL_VALUE_EXPR (decl, init);
-      DECL_HAS_VALUE_EXPR_P (decl) = 1;
       /* For decl_constant_var_p.  */
       DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl) = 1;
     }
@@ -4505,13 +4503,13 @@ cp_make_fname_decl (location_t loc, tree id, int type_dep)
     {
       DECL_CONTEXT (decl) = current_function_decl;
       decl = pushdecl_outermost_localscope (decl);
-      if (decl != error_mark_node)
-       add_decl_expr (decl);
+      cp_finish_decl (decl, init, /*init_const_expr_p=*/false, NULL_TREE,
+                     LOOKUP_ONLYCONVERTING);
     }
   else
     {
       DECL_THIS_STATIC (decl) = true;
-      pushdecl_top_level_and_finish (decl, NULL_TREE);
+      pushdecl_top_level_and_finish (decl, init);
     }

   return decl;
mikee47 commented 4 years ago

A better patch? The critical call appears to be DECL_INITIAL() to restore original behaviour.

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index b57ded813b6..9df54210953 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -4495,15 +4495,13 @@ cp_make_fname_decl (location_t loc, tree id, int type_dep)

   if (init)
     {
-      SET_DECL_VALUE_EXPR (decl, init);
-      DECL_HAS_VALUE_EXPR_P (decl) = 1;
+      DECL_INITIAL (decl) = init;
       /* For decl_constant_var_p.  */
       DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl) = 1;
     }

   if (current_function_decl)
     {
-      DECL_CONTEXT (decl) = current_function_decl;
       decl = pushdecl_outermost_localscope (decl);
       if (decl != error_mark_node)
        add_decl_expr (decl);
mikee47 commented 4 years ago

Note that the sections are no longer marked mergeable but not sure how important that is for these.