[Lldb-commits] [lldb] r182065 - Implement ObjectFileELF::GetModuleSpecifications(), and add PlatformLinux code to deal with unknown arch properties.

[Greg Clayton]

On May 17, 2013, at 10:24 AM, "Malea, Daniel" <daniel.malea at intel.com> wrote:

> Hi Mike, thanks for root-causing this issue. I studied the code in
> question a little and my comments inline below:
> 
> On 2013-05-17 12:58 AM, "Mike Sartain" <mikesart at valvesoftware.com> wrote:
> 
>> The below patch will fix up the vendor & os in
>> Target::SetExecutableModule() if either are unknown. I've got a couple of
>> questions / concerns though:
>> - The code below will run on all platforms. That scares me.
> 
> I don't think this is a problem because nothing the patch does in common
> code is OS-specific. If Target just fills in the blanks/unknowns, after
> the executable has been fully parsed, I don't think it will cause any
> problems. I agree that it's not pretty though :)

This code needs to be moved to the current platform. When debugging by default the native platform is selected, but you might select another platform (like "platform select remote-ios"). So we need to use the following lldb_private::Platform function:

        virtual bool
        IsCompatibleArchitecture (const ArchSpec &arch,
                                  bool exact_arch_match,
                                  ArchSpec *compatible_arch_ptr);


"arch" is the platform in question, and you will want to call this with "exact_arch_match" set to false. You pass in the address of the fixed architecture in "compatible_arch_ptr". On exit, if true is returned, "compatible_arch_ptr" will now contain a properly filled in architecture. The code in each platform will check for the vendor and OS not being filled in and fill them in appropriately if the platform can handle the architecture. For example, the "remote-ios" platform will only return true if the architecture is an ARM variant. For the linux host platform, it should only return true for x86_64 on 64 bit desktop PC hosts, and true for i386 on 32 bit desktop PC hosts (intel hosts that is)...

> 
> The only alternative I see is to beef up ObjectFileELF to read more than
> just the ELF header. Some quick inspection of some binaries with "readelf
> -a" seems to show the OS is encoded somewhere in the 'notes' section of
> the binary. I'm not sure what the overhead of reading those sections are,
> or if we want to go that far when we're just doing a process list. There
> might be another way to determine the Vendor/OS (the 'file' command seems
> to do it, so maybe there's a better way?) but I'm not sure it would be
> worth the trouble IMHO.

Anything that can be read from the binary to help figure out the vendor and OS would be great, but it would need to be bulletproof. 

> 
>> - This is the 2nd spot where we have to fix this arch stuff up for Linux.
>> This scares me a bit as well.
>> How big of a can of worms are we opening by returning
>> x86_64-unknown-unknown for elf files (since that's all the info we have
>> in ObjectFileELF), and then expect everyone to fix up the vendor/os later
>> on?

Again, if there is anything we can do in ObjectFileELF to fill out the triple better (looking for specific notes, section names, etc), then we should do that in addition to the other fixes. We do need to classify the target triple for the object files correctly because each ELF file creates its own AST. The AST is used to create types from the DWARF, so the triple really does need to be set correctly so when we create a new AST for an expression and copy type T from foo.elf and type Y from bar.elf, they can be imported correctly. It might be that the functions:

bool
ObjectFile::GetArchitecture(ArchSpec &arch); 

const ArchSpec& 
Module::GetArchitecture () const

They might need to also take a lldb_private::Target as an optional pointer parameter since. The current platform for a target can be extracted from the Target, and could be used to fill in the architecture correctly on a per target basis where we need it to be fully filled out with vendor and OS. So the new functions could be:


bool
ObjectFile::GetArchitecture(ArchSpec &arch, Target *target_ptr = NULL); 

ArchSpec
Module::GetArchitecture (Target *target_ptr = NULL) const

Then if the target is non NULL we can get the extra details from it.

> I'd like to think this is the only place we might have to do something
> like this. I believe (though not sure, maybe Apple guys can confirm?) that
> once the Target is initialized, everything that needs to query the
> architecture/vendor/OS should just use the target. As such I think your
> fix in Target.cpp is OK.


So Mike: the fix you had in Target.cpp should use the Platform:: IsCompatibleArchitecture() function to fully fill out the architecture if the vendor and OS are not specified using the platform that is compatible. 

The TargetList::CreateTarget does some platform calls to find a platform for the given architecture and select it correctly if possible. For example if only one platform supports a specific CPU type, we should select that platform automatically when "file foo" is called. This is another reason that the better we can fill out the target triple from the ObjectFile and target perspective, the better job we can do selecting all of the plug-ins (Process, Platform, Disassembler, DynamicLoader, OperatingSystem, etc).

> 
> Cheers,
> Dan
> 
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits