1416183 - mfbt/lz4.c:310:36: error: 'register' storage class specifier is deprecated and incompatible with C++17 [-Werror,-Wdeprecated-register]

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Christian Holler (:decoder)]

I got this error with Clang current trunk (r317840):

https://treeherder.mozilla.org/logviewer.html#?job_id=143643775&repo=try&lineNumber=3739

For now I can probably circumvent this with -Wno-deprecated-register but I guess we should eventually fix this properly.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

I have been working on it directly upstream:
https://github.com/lz4/lz4/pull/417
https://github.com/lz4/lz4/pull/419

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1416183 - Silent the -Wdeprecated-register until it is fixed in lz4 upstream

Review commit: https://reviewboard.mozilla.org/r/199026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199026/

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8927753 [details]
Bug 1416183 - Silent the -Wdeprecated-register until it is fixed in lz4 upstream

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199026/diff/1-2/

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8927753 [details]
Bug 1416183 - Silent the -Wdeprecated-register until it is fixed in lz4 upstream

https://reviewboard.mozilla.org/r/199026/#review204414

This is not the first time, and this is probably not the last time we have problems because lz4.c is built as part including it in a C++ file.

How about building it as C once and for all? It should be enough to remove the include in Compression.cpp, add a #include "lz4.h" instead (not in an anonymous namespace, obviously), and add lz4.c to SOURCES. (after bug 1416989 lands)

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1416183 - Build LZ4 as C instead of including it as C++.

Review commit: https://reviewboard.mozilla.org/r/199594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199594/

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8928389 [details]
Bug 1416183 - Build LZ4 as C instead of including it as C++.

https://reviewboard.mozilla.org/r/199594/#review204880

Thank you, this is going to make many things easier.

::: mfbt/Compression.cpp:16
(Diff revision 1)
> -using namespace mozilla::Compression;
> -
> -namespace {
> -
>  extern "C" {
> -
> +#include "lz4.h"

Are we sure this change from anonymous namespace to normal C file doesn't export more functions from libxul?

::: mfbt/moz.build
(Diff revision 1)
> -if CONFIG['GNU_CXX']:
> -    SOURCES['Compression.cpp'].flags += ['-Wno-unused-function']
> -    CXXFLAGS += ['-Wno-error=shadow']

Presumably the first one is no longer needed now that we compile lz4.c as a separate file...is the second one not needed for similar reasons?

::: mfbt/moz.build
(Diff revision 1)
> -if CONFIG['_MSC_VER']:
> -    # Error 4804 is "'>' : unsafe use of type 'bool' in operation"
> -    SOURCES['Compression.cpp'].flags += ['-wd4804']

...and I assume we're applying slightly different rules for C and C++ here as well, so we don't need to disable this warning for C?

Comment 7

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> Comment on attachment 8928389 [details]
> Bug 1416183 - Build LZ4 as C instead of including it as C++.
> 
> https://reviewboard.mozilla.org/r/199594/#review204880
> 
> Thank you, this is going to make many things easier.
> 
> ::: mfbt/Compression.cpp:16
> (Diff revision 1)
> > -using namespace mozilla::Compression;
> > -
> > -namespace {
> > -
> >  extern "C" {
> > -
> > +#include "lz4.h"
> 
> Are we sure this change from anonymous namespace to normal C file doesn't
> export more functions from libxul?

damn, I thought I had looked but now looking again, there's a LZ4LIB_API that sets the visibility to default :(

(there's also an automatic extern "C", we could remote the wrapping in Compression.cpp)

Newer versions of the file actually allow to override that with LZ4LIB_VISIBILITY. Let's go with a lz4.h patch that is compatible with that.

> ::: mfbt/moz.build
> (Diff revision 1)
> > -if CONFIG['GNU_CXX']:
> > -    SOURCES['Compression.cpp'].flags += ['-Wno-unused-function']
> > -    CXXFLAGS += ['-Wno-error=shadow']
> 
> Presumably the first one is no longer needed now that we compile lz4.c as a
> separate file...is the second one not needed for similar reasons?
> 
> ::: mfbt/moz.build
> (Diff revision 1)
> > -if CONFIG['_MSC_VER']:
> > -    # Error 4804 is "'>' : unsafe use of type 'bool' in operation"
> > -    SOURCES['Compression.cpp'].flags += ['-wd4804']
> 
> ...and I assume we're applying slightly different rules for C and C++ here
> as well, so we don't need to disable this warning for C?

I'm not sure why, but I just tried and it builds just fine on all platforms without the flags. Maybe they were wranings that were due to compiling C code as C++.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8928389 [details]
Bug 1416183 - Build LZ4 as C instead of including it as C++.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199594/diff/1-2/

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8928389 [details]
Bug 1416183 - Build LZ4 as C instead of including it as C++.

https://reviewboard.mozilla.org/r/199594/#review205120

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f31b7226512
Build LZ4 as C instead of including it as C++. r=froydnj

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/6f31b7226512

Comment 12

[Gerry Chang [:gchang]]

Won't fix for 58. Let it ride the train.