[LTP] LTP compilation broken with -fno-common

[LTP] LTP compilation broken with -fno-common

[Petr Vorel]

Hi,

LTP compilation breaks with -fno-common (we don't use as the default).
This is the default for GCC 10, is there a way to fix it or do we need to disable it?

Compilation on gcc-10 (or gcc-9 with CFLAGS="-fno-common":

gcc-9 -Werror=implicit-function-declaration -fno-common -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -W  -I../../include -I../../include -I../../include/old/   -L../../lib  tst_checkpoint_wait_timeout.c   -lltp -o tst_checkpoint_wait_timeout
/usr/lib64/gcc/x86_64-suse-linux/9/../../../../x86_64-suse-linux/bin/ld: ../../lib/libltp.a(tst_test.o):ltp.git/lib/../include/tst_test.h:325: multiple definition of `TCID'; /tmp/cc49yYO6.o:ltp.git/lib/tests/tst_strsig.c:27: first defined here
/usr/lib64/gcc/x86_64-suse-linux/9/../../../../x86_64-suse-linux/bin/ld: ../../lib/libltp.a(tst_sys_conf.o):ltp.git/lib/../include/tst_test.h:325: multiple definition of `TCID'; /tmp/cc49yYO6.o:ltp.git/lib/tests/tst_strsig.c:27: first defined here

It's somehow related to the "hack" in include/tst_test.h:

/*
 * This is a hack to make the testcases link without defining TCID
 */
extern const char *TCID;

Kind regards,
Petr

[LTP] LTP compilation broken with -fno-common

[Richard Palethorpe]

Hello,

Petr Vorel <pvorel@suse.cz> writes:

> Hi,
>
> LTP compilation breaks with -fno-common (we don't use as the default).
> This is the default for GCC 10, is there a way to fix it or do we need to disable it?
>
> Compilation on gcc-10 (or gcc-9 with CFLAGS="-fno-common":
>
> gcc-9 -Werror=implicit-function-declaration -fno-common -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -W  -I../../include -I../../include -I../../include/old/   -L../../lib  tst_checkpoint_wait_timeout.c   -lltp -o tst_checkpoint_wait_timeout
> /usr/lib64/gcc/x86_64-suse-linux/9/../../../../x86_64-suse-linux/bin/ld: ../../lib/libltp.a(tst_test.o):ltp.git/lib/../include/tst_test.h:325: multiple definition of `TCID'; /tmp/cc49yYO6.o:ltp.git/lib/tests/tst_strsig.c:27: first defined here
> /usr/lib64/gcc/x86_64-suse-linux/9/../../../../x86_64-suse-linux/bin/ld: ../../lib/libltp.a(tst_sys_conf.o):ltp.git/lib/../include/tst_test.h:325: multiple definition of `TCID'; /tmp/cc49yYO6.o:ltp.git/lib/tests/tst_strsig.c:27: first defined here
>
> It's somehow related to the "hack" in include/tst_test.h:
>
> /*
>  * This is a hack to make the testcases link without defining TCID
>  */
> extern const char *TCID;

Huh? This is defining the variable and with extern which looks correct
(at a glance):
https://gcc.gnu.org/gcc-10/porting_to.html

Possibly we need to clean the code up so this variable is not redefined
all over the place?
>
> Kind regards,
> Petr


-- 
Thank you,
Richard.

[LTP] LTP compilation broken with -fno-common

[Cyril Hrubis]

Hi!
> LTP compilation breaks with -fno-common (we don't use as the default).
> This is the default for GCC 10, is there a way to fix it or do we need to disable it?
> 
> Compilation on gcc-10 (or gcc-9 with CFLAGS="-fno-common":
> 
> gcc-9 -Werror=implicit-function-declaration -fno-common -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -W  -I../../include -I../../include -I../../include/old/   -L../../lib  tst_checkpoint_wait_timeout.c   -lltp -o tst_checkpoint_wait_timeout
> /usr/lib64/gcc/x86_64-suse-linux/9/../../../../x86_64-suse-linux/bin/ld: ../../lib/libltp.a(tst_test.o):ltp.git/lib/../include/tst_test.h:325: multiple definition of `TCID'; /tmp/cc49yYO6.o:ltp.git/lib/tests/tst_strsig.c:27: first defined here
> /usr/lib64/gcc/x86_64-suse-linux/9/../../../../x86_64-suse-linux/bin/ld: ../../lib/libltp.a(tst_sys_conf.o):ltp.git/lib/../include/tst_test.h:325: multiple definition of `TCID'; /tmp/cc49yYO6.o:ltp.git/lib/tests/tst_strsig.c:27: first defined here
> 
> It's somehow related to the "hack" in include/tst_test.h:
> 
> /*
>  * This is a hack to make the testcases link without defining TCID
>  */
> extern const char *TCID;

This is not the original line, the original line is without the extern.

Now this will not be easy to solve since the problem is:

* All old style test define TCID in the test source that also implements main()

* The test library uses that variable in various places

* The variable definition is in the tst_test.h so that all new test also
  define TCID

We cannot move that definition into tst_test.c beacuse that would mean
that old API test would end up with multiple definitions one in the
corresponding test and one in the new library library code.

Maybe we can add it with a weak linker attribute. What about this patch:

diff --git a/include/tst_test.h b/include/tst_test.h
index 259d87d60..352e2cd64 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -322,9 +322,7 @@ int main(int argc, char *argv[])
 
 #define TST_TEST_TCONF(message)                                 \
         static struct tst_test test = { .tconf_msg = message  } \
-/*
- * This is a hack to make the testcases link without defining TCID
- */
-const char *TCID;
+
+extern const char *TCID;
 
 #endif /* TST_TEST_H__ */
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 73827d472..4ce8cc301 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -32,6 +32,11 @@
 #include "old_device.h"
 #include "old_tmpdir.h"
 
+/*
+ * Hack to get TCID defined in newlib tests
+ */
+const char *TCID __attribute__((weak));
+
 #define LINUX_GIT_URL "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id="
 #define CVE_DB_URL "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-"



I guess that we can even drop the TCID extern definition from the tst_test.h
header if the weak attribute works fine.


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] LTP compilation broken with -fno-common

[Martin Doucha]

On 01. 04. 20 11:39, Cyril Hrubis wrote:
> We cannot move that definition into tst_test.c beacuse that would mean
> that old API test would end up with multiple definitions one in the
> corresponding test and one in the new library library code.

But we can move it a few lines up in tst_main.h so that it gets defined
only together with the default main(), not in every .c file that
includes tst_test.h.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] LTP compilation broken with -fno-common

[Cyril Hrubis]

Hi!
> > We cannot move that definition into tst_test.c beacuse that would mean
> > that old API test would end up with multiple definitions one in the
> > corresponding test and one in the new library library code.
> 
> But we can move it a few lines up in tst_main.h so that it gets defined
> only together with the default main(), not in every .c file that
> includes tst_test.h.

The problem here is that gcc10 forbids variables in headers, moving it
anywhere int the header wouldn't help.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] LTP compilation broken with -fno-common

[Martin Doucha]

On 01. 04. 20 11:58, Cyril Hrubis wrote:
> The problem here is that gcc10 forbids variables in headers, moving it
> anywhere int the header wouldn't help.

I don't think GCC cares about where exactly the variable came from. It's
only refusing to link multiple definitions of the same symbol. So if we
move the TCID definition into the TST_NO_DEFAULT_MAIN guard block, GCC
linker should stop complaining because the symbol name will become unique.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] LTP compilation broken with -fno-common

[Petr Vorel]

Hi Martin,

> On 01. 04. 20 11:58, Cyril Hrubis wrote:
> > The problem here is that gcc10 forbids variables in headers, moving it
> > anywhere int the header wouldn't help.

> I don't think GCC cares about where exactly the variable came from. It's
> only refusing to link multiple definitions of the same symbol. So if we
> move the TCID definition into the TST_NO_DEFAULT_MAIN guard block, GCC
> linker should stop complaining because the symbol name will become unique.

No, moving the definition above #ifndef TST_NO_DEFAULT_MAIN does not help.

Kind regards,
Petr

[LTP] LTP compilation broken with -fno-common

[Cyril Hrubis]

Hi!
> > The problem here is that gcc10 forbids variables in headers, moving it
> > anywhere int the header wouldn't help.
> 
> I don't think GCC cares about where exactly the variable came from. It's
> only refusing to link multiple definitions of the same symbol. So if we
> move the TCID definition into the TST_NO_DEFAULT_MAIN guard block, GCC
> linker should stop complaining because the symbol name will become unique.

See:

https://gcc.gnu.org/gcc-10/porting_to.html

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] LTP compilation broken with -fno-common

[Martin Doucha]

On 01. 04. 20 12:50, Cyril Hrubis wrote:
> Hi!
>>> The problem here is that gcc10 forbids variables in headers, moving it
>>> anywhere int the header wouldn't help.
>>
>> I don't think GCC cares about where exactly the variable came from. It's
>> only refusing to link multiple definitions of the same symbol. So if we
>> move the TCID definition into the TST_NO_DEFAULT_MAIN guard block, GCC
>> linker should stop complaining because the symbol name will become unique.
> 
> See:
> 
> https://gcc.gnu.org/gcc-10/porting_to.html
> 

This patch fixes the "multiple definition of `TCID'" error. But never
mind because we'll get the "undefined reference to `TCID'" error instead
from about 50 programs files that use the new library but define their
own main().

Reminder that "multiple definition of `TCID'" is a linker error. If
defining variables in .h files were really forbidden, it'd be a
preprocessor error instead.

diff --git a/include/tst_test.h b/include/tst_test.h
index 42c02b549..cc163ae51 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -308,6 +308,10 @@ char *tst_get_tmpdir(void);

 #ifndef TST_NO_DEFAULT_MAIN

+/*
+ * This is a hack to make the testcases link without defining TCID
+ */
+const char *TCID;
 static struct tst_test test;

 int main(int argc, char *argv[])
@@ -319,9 +323,7 @@ int main(int argc, char *argv[])

 #define TST_TEST_TCONF(message)                                 \
         static struct tst_test test = { .tconf_msg = message  } \
-/*
- * This is a hack to make the testcases link without defining TCID
- */
-const char *TCID;
+
+extern const char *TCID;

 #endif /* TST_TEST_H__ */


-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] LTP compilation broken with -fno-common

[Cyril Hrubis]

Hi!
> >>> The problem here is that gcc10 forbids variables in headers, moving it
> >>> anywhere int the header wouldn't help.
> >>
> >> I don't think GCC cares about where exactly the variable came from. It's
> >> only refusing to link multiple definitions of the same symbol. So if we
> >> move the TCID definition into the TST_NO_DEFAULT_MAIN guard block, GCC
> >> linker should stop complaining because the symbol name will become unique.
> > 
> > See:
> > 
> > https://gcc.gnu.org/gcc-10/porting_to.html
> > 
> 
> This patch fixes the "multiple definition of `TCID'" error. But never
> mind because we'll get the "undefined reference to `TCID'" error instead
> from about 50 programs files that use the new library but define their
> own main().
> 
> Reminder that "multiple definition of `TCID'" is a linker error. If
> defining variables in .h files were really forbidden, it'd be a
> preprocessor error instead.

Well variables in headers are not exactly forbidden by -fno-common but
they are not resolved as a single global variable by the linker anymore,
which was the default before gcc10. So effectivelly this disallows
having variables in headers if the header is included from more than one
compilation unit becuase you end up with a linker error.

The end result is overall the same, no more variables in headers in any
non-trivial codebase.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] LTP compilation broken with -fno-common

[Petr Vorel]

Hi Cyril,

...
> Maybe we can add it with a weak linker attribute. What about this patch:

> diff --git a/include/tst_test.h b/include/tst_test.h
> index 259d87d60..352e2cd64 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -322,9 +322,7 @@ int main(int argc, char *argv[])

>  #define TST_TEST_TCONF(message)                                 \
>          static struct tst_test test = { .tconf_msg = message  } \
> -/*
> - * This is a hack to make the testcases link without defining TCID
> - */
> -const char *TCID;
> +
> +extern const char *TCID;

>  #endif /* TST_TEST_H__ */
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 73827d472..4ce8cc301 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -32,6 +32,11 @@
>  #include "old_device.h"
>  #include "old_tmpdir.h"

> +/*
> + * Hack to get TCID defined in newlib tests
> + */
> +const char *TCID __attribute__((weak));
> +
>  #define LINUX_GIT_URL "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id="
>  #define CVE_DB_URL "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-"

Yep working locally :).
https://travis-ci.org/github/pevik/ltp/builds/669619899

> I guess that we can even drop the TCID extern definition from the tst_test.h
> header if the weak attribute works fine.
Also this is working.
Let's wait for travis (also added -fno-common into CFLAGS for all jobs).
https://travis-ci.org/github/pevik/ltp/builds/669620839

Kind regards,
Petr

[LTP] LTP compilation broken with -fno-common

[Petr Vorel]

Hi all,

> > /*
> >  * This is a hack to make the testcases link without defining TCID
> >  */
> > extern const char *TCID;

> This is not the original line, the original line is without the extern.

> Now this will not be easy to solve since the problem is:

> * All old style test define TCID in the test source that also implements main()

> * The test library uses that variable in various places

> * The variable definition is in the tst_test.h so that all new test also
>   define TCID

> We cannot move that definition into tst_test.c beacuse that would mean
> that old API test would end up with multiple definitions one in the
> corresponding test and one in the new library library code.

> Maybe we can add it with a weak linker attribute. What about this patch:

> diff --git a/include/tst_test.h b/include/tst_test.h
> index 259d87d60..352e2cd64 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -322,9 +322,7 @@ int main(int argc, char *argv[])

>  #define TST_TEST_TCONF(message)                                 \
>          static struct tst_test test = { .tconf_msg = message  } \
> -/*
> - * This is a hack to make the testcases link without defining TCID
> - */
> -const char *TCID;
> +
> +extern const char *TCID;

>  #endif /* TST_TEST_H__ */
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 73827d472..4ce8cc301 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -32,6 +32,11 @@
>  #include "old_device.h"
>  #include "old_tmpdir.h"

> +/*
> + * Hack to get TCID defined in newlib tests
> + */
> +const char *TCID __attribute__((weak));
> +
>  #define LINUX_GIT_URL "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id="
>  #define CVE_DB_URL "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-"



> I guess that we can even drop the TCID extern definition from the tst_test.h
> header if the weak attribute works fine.

I'd be for adding this patch (both version works), Cyril, will you send it
to ML / push it?

I'd also be for adding -fno-common to Travis after this fix.

Kind regards,
Petr

[LTP] LTP compilation broken with -fno-common

[Cyril Hrubis]

Hi!
> > I guess that we can even drop the TCID extern definition from the tst_test.h
> > header if the weak attribute works fine.
> 
> I'd be for adding this patch (both version works), Cyril, will you send it
> to ML / push it?

Feel free do that yourself. I would be for the version without extern
definition in tst_test.h then.

> I'd also be for adding -fno-common to Travis after this fix.

Sounds good.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] LTP compilation broken with -fno-common

[Li Wang]

Hi Petr,

Petr Vorel <pvorel@suse.cz> wrote:

...
> > I guess that we can even drop the TCID extern definition from the
> tst_test.h
> > header if the weak attribute works fine.
>
> I'd be for adding this patch (both version works), Cyril, will you send it
> to ML / push it?
>
> I'd also be for adding -fno-common to Travis after this fix.
>

After building LTP with CFLAGS="-fno-common" locally, I just find more
places that need to do the same improvement.

    ltp/testcases/kernel/controllers/libcontrollers/libcontrollers.h:56:
multiple definition of `num_line'
    ltp/lib/../include/tst_hugepage.h:16: multiple definition of `Hopt'

Sorry for involving the definition in tst_hugetlb.h without 'extern' issue
many hours ago. It'd be appreciated if you can fix them together in your
coming patch.

Hope this is a timely reminder in case you formatting patch V2 :).

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200409/10c1c32e/attachment.htm>

[LTP] LTP compilation broken with -fno-common

[Petr Vorel]

Hi Li,

> After building LTP with CFLAGS="-fno-common" locally, I just find more
> places that need to do the same improvement.

>     ltp/testcases/kernel/controllers/libcontrollers/libcontrollers.h:56:
> multiple definition of `num_line'
>     ltp/lib/../include/tst_hugepage.h:16: multiple definition of `Hopt'

> Sorry for involving the definition in tst_hugetlb.h without 'extern' issue
> many hours ago. It'd be appreciated if you can fix them together in your
> coming patch.

> Hope this is a timely reminder in case you formatting patch V2 :).
Thanks for info, working on the fix :).

Kind regards,
Petr

[LTP] hugemmap01 warnings with -i [was Re: LTP compilation broken with -fno-common]

[Petr Vorel]

Hi Li,

> After building LTP with CFLAGS="-fno-common" locally, I just find more
> places that need to do the same improvement.

>     ltp/testcases/kernel/controllers/libcontrollers/libcontrollers.h:56:
> multiple definition of `num_line'
>     ltp/lib/../include/tst_hugepage.h:16: multiple definition of `Hopt'

> Sorry for involving the definition in tst_hugetlb.h without 'extern' issue
> many hours ago. It'd be appreciated if you can fix them together in your
> coming patch.
BTW that's a fix 156f91396 ("hugetlb: move nr_opt to tst_hugepage.h")

I haven't checked whether -i worked without warning before, but not it
produces it:

./hugemmap01  -s 2 -i 2
tst_hugepage.c:48: INFO: 2 hugepage(s) reserved
tst_test.c:1244: INFO: Timeout per run is 0h 05m 00s
hugemmap01.c:61: PASS: call succeeded
hugemmap01.c:61: PASS: call succeeded
hugemmap01.c:69: WARN: Number of HUGEPAGES_FREE stayed the same. Okay if multiple copies running due to test collision.

Kind regards,
Petr

[LTP] hugemmap01 warnings with -i [was Re: LTP compilation broken with -fno-common]

[Li Wang]

Hi Petr,

On Wed, Apr 15, 2020 at 6:20 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Li,
>
> > After building LTP with CFLAGS="-fno-common" locally, I just find more
> > places that need to do the same improvement.
>
> >     ltp/testcases/kernel/controllers/libcontrollers/libcontrollers.h:56:
> > multiple definition of `num_line'
> >     ltp/lib/../include/tst_hugepage.h:16: multiple definition of `Hopt'
>
> > Sorry for involving the definition in tst_hugetlb.h without 'extern'
> issue
> > many hours ago. It'd be appreciated if you can fix them together in your
> > coming patch.
> BTW that's a fix 156f91396 ("hugetlb: move nr_opt to tst_hugepage.h")
>
> I haven't checked whether -i worked without warning before, but not it
> produces it:
>

This is not introduced by hugetlb recent fixes, the warning always exists
there in tag ltp-full-20200120.
Anyway, it's a tiny issue and I will have a look when available :).

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200416/1abdc025/attachment-0001.htm>