Page MenuHomePhabricator

[LLDB][ppc64le] Rename enums in AuxVector
ClosedPublic

Authored by brunoalr on Jul 6 2017, 9:21 AM.

Details

Summary

On linux on ppc64le some of the enums in AuxVector have the same name
as macros defined in the system.

Diff Detail

Repository
rL LLVM

Event Timeline

brunoalr created this revision.Jul 6 2017, 9:21 AM

Adding Code Owner (Pavel Labeth) and responsible for the relevant changes (Michael Sartain).

brunoalr retitled this revision from Rename enums in AuxVector to [LLDB][ppc64le] Rename enums in AuxVector.Jul 6 2017, 9:27 AM

Values like AT_NULL are macros on NetBSD and there are no problems?

Values like AT_NULL are macros on NetBSD and there are no problems?

I haven't tested in ppc64le on NetBSD, but I'm afraid the same problems would occur.

gut added a subscriber: gut.Jul 6 2017, 9:41 AM

What are the build failures?

joerg added a subscriber: joerg.Jul 6 2017, 9:56 AM

If you want to go this way, rename them consistently and use a different prefix (e.g. AUXV_*) please.

labath added inline comments.Jul 6 2017, 10:17 AM
source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.cpp
11 ↗(On Diff #105454)

Do you still get the error if you remove these includes?

As far as I can tell they are unused, and this part of the code should not depend on system headers anyway.

If that doesn't help, then we should use a different prefix as joerg suggests.

115 ↗(On Diff #105454)

If we go about renaming them, then we should change this, as we still want to display the standard name of the entries.

What are the build failures?

[2397/3183] Building CXX object tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/CMakeFiles/lldbPluginDynamicLoaderPosixDYLD.dir/AuxVector.cpp.o
FAILED: tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/CMakeFiles/lldbPluginDynamicLoaderPosixDYLD.dir/AuxVector.cpp.o
/usr/bin/c++   -DGTEST_HAS_RTTI=0 -DHAVE_ROUND -DLIBXML2_DEFINED -DLLDB_USE_BUILTIN_DEMANGLER -DLLVM_BUILD_GLOBAL_ISEL -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIM
IT_MACROS -Itools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD -I/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD -Itools/lldb/include -I/home/brosa/llvm/tools/lldb/include -Iinclude
-I/home/brosa/llvm/include -I/usr/include/python2.7 -I/home/brosa/llvm/tools/clang/include -Itools/lldb/../clang/include -I/usr/include/libxml2 -I/home/brosa/llvm/tools/lldb/source/. -fPIC -fvisibility-
inlines-hidden -Werror=date-time -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virt
ual-dtor -Wno-comment -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-deprecated-register -Wno-vla-extension -g    -fno-exceptions -fno-rtti -MD -MT tools/lldb/source/Plugins
/DynamicLoader/POSIX-DYLD/CMakeFiles/lldbPluginDynamicLoaderPosixDYLD.dir/AuxVector.cpp.o -MF tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/CMakeFiles/lldbPluginDynamicLoaderPosixDYLD.dir/AuxVector
.cpp.o.d -o tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/CMakeFiles/lldbPluginDynamicLoaderPosixDYLD.dir/AuxVector.cpp.o -c /home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxV
ector.cpp
In file included from /usr/include/powerpc64le-linux-gnu/asm/elf.h:17:0,
                 from /usr/include/powerpc64le-linux-gnu/asm/sigcontext.h:13,
                 from /usr/include/powerpc64le-linux-gnu/bits/sigcontext.h:27,
                 from /usr/include/signal.h:306,
                 from /home/brosa/llvm/tools/lldb/include/lldb/lldb-types.h:17,
                 from /home/brosa/llvm/tools/lldb/include/lldb/lldb-private-interfaces.h:17,
                 from /home/brosa/llvm/tools/lldb/include/lldb/lldb-private.h:17,
                 from /home/brosa/llvm/tools/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h:21,
                 from /home/brosa/llvm/tools/lldb/include/lldb/Breakpoint/BreakpointSite.h:22,
                 from /home/brosa/llvm/tools/lldb/include/lldb/Breakpoint/BreakpointSiteList.h:21,
                 from /home/brosa/llvm/tools/lldb/include/lldb/Target/Process.h:29,
                 from /home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.cpp:17:
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:65:5: error: expected identifier before numeric constant
     AT_DCACHEBSIZE = 19,   ///< Data cache block size.
     ^
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:65:5: error: expected ‘}’ before numeric constant
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:65:5: error: expected unqualified-id before numeric constant
In file included from /home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.cpp:26:0:
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:82:1: error: expected unqualified-id before ‘private’
 private:
 ^~~~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:85:1: error: expected unqualified-id before ‘public’
 public:
 ^~~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:88:3: error: ‘iterator’ does not name a type
   iterator begin() const { return m_auxv.begin(); }
   ^~~~~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:89:3: error: ‘iterator’ does not name a type
   iterator end() const { return m_auxv.end(); }
   ^~~~~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:91:3: error: ‘iterator’ does not name a type
   iterator FindEntry(EntryType type) const;
   ^~~~~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:93:41: error: ‘Entry’ does not name a type
   static const char *GetEntryName(const Entry &entry) {
                                         ^~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h: In function ‘const char* GetEntryName(const int&)’:
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:94:37: error: ‘EntryType’ does not name a type
     return GetEntryName(static_cast<EntryType>(entry.type));
                                     ^~~~~~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:94:54: error: request for member ‘type’ in ‘entry’, which is of non-class type ‘const int’
     return GetEntryName(static_cast<EntryType>(entry.type));
                                                      ^~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h: At global scope:
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:97:35: error: ‘const char* GetEntryName’ redeclared as different kind of symbol
   static const char *GetEntryName(EntryType type);
                                   ^~~~~~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:93:22: note: previous declaration ‘const char* GetEntryName(const int&)’
   static const char *GetEntryName(const Entry &entry) {
                      ^~~~~~~~~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:97:35: error: ‘EntryType’ was not declared in this scope
   static const char *GetEntryName(EntryType type);
                                   ^~~~~~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:99:42: error: non-member function ‘void DumpToLog(lldb_private::Log*)’ cannot have cv-qualifier
   void DumpToLog(lldb_private::Log *log) const;
                                          ^~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:101:1: error: expected unqualified-id before ‘private’
 private:
 ^~~~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:103:3: error: ‘EntryVector’ does not name a type
   EntryVector m_auxv;
   ^~~~~~~~~~~
/home/brosa/llvm/tools/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h:108:1: error: expected declaration before ‘}’ token
 };
 ^
brunoalr marked an inline comment as done.Jul 6 2017, 10:51 AM
brunoalr added inline comments.
source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.cpp
11 ↗(On Diff #105454)

I just tried it and it doesn't help, unfortunately.

brunoalr marked 2 inline comments as done.EditedJul 6 2017, 10:53 AM

If you want to go this way, rename them consistently and use a different prefix (e.g. AUXV_*) please.

Will do.

brunoalr updated this revision to Diff 105514.Jul 6 2017, 1:06 PM

Renaming enum consistently and updating name ENTRY_NAME macro to
correctly assign the standard names of aux vector entries.

brunoalr updated this revision to Diff 105515.Jul 6 2017, 1:09 PM

Fixing replace mistake.

brunoalr marked an inline comment as done.Jul 6 2017, 1:10 PM
brunoalr updated this revision to Diff 105517.Jul 6 2017, 1:27 PM

Add comments

joerg added inline comments.Jul 6 2017, 2:54 PM
source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.cpp
115 ↗(On Diff #105454)

Or name = "AUXV_AT_???" ENTRY_NAME(type) name = #type and then return name + 4;

source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h
46 ↗(On Diff #105517)

I think most targets do, but they don't pull the relevant system headers in via namespace pollution.

brunoalr added inline comments.Jul 6 2017, 2:58 PM
source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h
46 ↗(On Diff #105517)

Should I change this comment to

/// Added AUXV prefix to avoid potential conflicts with system-defined MACROS

?

brunoalr added inline comments.Jul 6 2017, 3:06 PM
source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.cpp
115 ↗(On Diff #105454)

Hmm that looks too "hacky", doesn't it?

krytarowski added inline comments.Jul 6 2017, 3:10 PM
source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.cpp
115 ↗(On Diff #105454)

name + 4 looks prettier to me.

joerg added inline comments.Jul 6 2017, 4:09 PM
source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.cpp
115 ↗(On Diff #105454)

Just using the name directly makes this searchable. The cleanup is an implementation detail, even if it might waste a bit space.

source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h
46 ↗(On Diff #105517)

Yes, please.

labath added inline comments.Jul 7 2017, 1:17 AM
source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.cpp
115 ↗(On Diff #105454)

I think name+4 (well, 5) is fine, but if you want to keep it this way, then I'd suggest you hardcode the AUXV_ prefix into the macro (#define ENTRY_NAME(type) AUXV_##type: name = #type. This is just adds redundant magic.

brunoalr updated this revision to Diff 105638.Jul 7 2017, 7:11 AM

Applying suggestions from review

brunoalr updated this revision to Diff 105639.Jul 7 2017, 7:15 AM

Use string+5 instead of concatenation

brunoalr updated this revision to Diff 105640.Jul 7 2017, 7:18 AM

Small fix

brunoalr marked 8 inline comments as done.Jul 7 2017, 9:39 AM
labath accepted this revision.Jul 10 2017, 4:17 AM

cool. thanks

This revision is now accepted and ready to land.Jul 10 2017, 4:17 AM
brunoalr added a comment.EditedJul 10 2017, 6:23 AM

@labath @joerg @krytarowski thanks for the review. Can anyone commit this, please? I still don't have commit privileges.

This revision was automatically updated to reflect the committed changes.

@labath @joerg @krytarowski thanks for the review. Can anyone commit this, please? I still don't have commit privileges.

thanks, @labath