This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][MIPS] Getting correct flags for MIPS
ClosedPublic

Authored by nitesh.jain on Jun 24 2015, 3:35 AM.

Details

Summary

As per the discussions, updated signature for "ConvertMmapFlagsToPlatform" method to take "ArchSpec" as additional parameter.

Diff Detail

Repository
rL LLVM

Event Timeline

nitesh.jain retitled this revision from to [LLDB][MIPS] Getting correct flags for MIPS.
nitesh.jain updated this object.
nitesh.jain edited the test plan for this revision. (Show Details)
nitesh.jain added reviewers: clayborg, ovyalov.
nitesh.jain set the repository for this revision to rL LLVM.
ovyalov added inline comments.Jun 24 2015, 2:32 PM
source/Plugins/Platform/Linux/PlatformLinux.cpp
940 ↗(On Diff #28328)

Please add spaces around equal sign.

941 ↗(On Diff #28328)

We may need a special care for remote debugging scenario - i.e. use HostInfoBase::GetArchitecture() if IsHost() otherwise get archspec from remote target via GetRemoteSystemArchitecture

tberghammer added inline comments.
source/Plugins/Platform/Linux/PlatformLinux.cpp
941 ↗(On Diff #28328)

We should use GetSystemArchitecture() what do this (with some additional caching)

clayborg requested changes to this revision.Jun 25 2015, 10:14 AM
clayborg edited edge metadata.

We probably need to pass in an architecture:

virtual uint64_t
Platform:: ConvertMmapFlagsToPlatform (const ArchSpec &arch, unsigned flags);

Then have all callers specify an architecture.

source/Plugins/Platform/Linux/PlatformLinux.cpp
941 ↗(On Diff #28328)

You can't just get the Host for its architecture. We might need to change this function to take an "const ArchSpec &arch" as an argument and have the caller specify the right architecture using the current target.

This revision now requires changes to proceed.Jun 25 2015, 10:14 AM
nitesh.jain updated this object.Jun 29 2015, 5:55 AM
nitesh.jain edited edge metadata.
nitesh.jain updated this object.
nitesh.jain edited edge metadata.
labath added a subscriber: labath.Jun 29 2015, 6:29 AM
labath added inline comments.
source/Target/Platform.cpp
1512 ↗(On Diff #28665)

Wouldn't this be better in PlatformLinux? What are the odds of this constant being correct on non-linux platforms?

emaste requested changes to this revision.Jun 29 2015, 7:10 AM
emaste added a reviewer: emaste.
emaste added a subscriber: emaste.
emaste added inline comments.
source/Target/Platform.cpp
1512 ↗(On Diff #28665)

Yes, this is incorrect on FreeBSD.

/usr/include/sys/mman.h
84:#define MAP_ANON 0x1000 /* allocated from memory, swap space */
86:#define MAP_ANONYMOUS MAP_ANON /* For compatibility. */

This revision now requires changes to proceed.Jun 29 2015, 7:10 AM
nitesh.jain edited edge metadata.
clayborg accepted this revision.Jun 29 2015, 11:35 AM
clayborg edited edge metadata.

Looks good.

ovyalov accepted this revision.Jun 29 2015, 12:02 PM
ovyalov edited edge metadata.

LGTM

This revision was automatically updated to reflect the committed changes.