[PATCH] D131347: Set up basic infrastructure for ELF/i386 backend support in JITLink .

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

@tschuett's suggestion is a good one.

Otherwise LGTM. Just let me know what name and email you would like me to use for attribution and I can land this tomorrow. :)



================
Comment at: llvm/test/ExecutionEngine/JITLink/i386/ELF_i386_minimal.s:3
+# RUN: llvm-mc -triple=i386-unknown-linux-gnu -position-independent -filetype=obj -o %t/i386_simple.o %s
+# RUN: llvm-jitlink %t/i386_simple.o; result=$?; [ $result -eq 42 ] || exit 1
+
----------------
jain98 wrote:
> jain98 wrote:
> > lhames wrote:
> > > Tests should be `-noexec` (except in special circumstances): We want them to be able to run on architectures other than the target.
> > > 
> > > I would simplify to:
> > > ```
> > > # RUN: llvm-mc -triple=i386-unknown-linux-gnu -position-independent -filetype=obj -o %t.o %s
> > > # RUN: llvm-jitlink -noexec %t.o
> > > ```
> > Does this also mean that the contents of this directory's `lit.local.cfg` should be changed? Currently this is what's in there - 
> > 
> > ```
> > if not 'i386' in config.root.targets:
> >   config.unsupported = True
> > ```
> > 
> > I may be wrong, but I //think// it's saying run this test only if the current host architecture is i386?
> > 
> Additionally, what the reasoning behind making these tests `--noexec` - it looks like most JitLink tests are? I'm wondering what makes the code in these tests non-runnable if we're able to load it into memory? I can see how the generated code for one architecture might be straight up rubbish for some other architecture, and therefore it does not make sense to actually try and execute the generated code. But then why go to the point where we're loading things into memory even? Why not stop after generating the `LinkGraph`?
> I may be wrong, but I think it's saying run this test only if the current host architecture is i386?

That check is saying "if this LLVM build isn't configured with i386 backend support". You want to keep it -- it tells us whether or not the tools can handle i386 assembly. :) 


================
Comment at: llvm/test/ExecutionEngine/JITLink/i386/ELF_i386_minimal.s:3
+# RUN: llvm-mc -triple=i386-unknown-linux-gnu -position-independent -filetype=obj -o %t/i386_simple.o %s
+# RUN: llvm-jitlink %t/i386_simple.o; result=$?; [ $result -eq 42 ] || exit 1
+
----------------
lhames wrote:
> jain98 wrote:
> > jain98 wrote:
> > > lhames wrote:
> > > > Tests should be `-noexec` (except in special circumstances): We want them to be able to run on architectures other than the target.
> > > > 
> > > > I would simplify to:
> > > > ```
> > > > # RUN: llvm-mc -triple=i386-unknown-linux-gnu -position-independent -filetype=obj -o %t.o %s
> > > > # RUN: llvm-jitlink -noexec %t.o
> > > > ```
> > > Does this also mean that the contents of this directory's `lit.local.cfg` should be changed? Currently this is what's in there - 
> > > 
> > > ```
> > > if not 'i386' in config.root.targets:
> > >   config.unsupported = True
> > > ```
> > > 
> > > I may be wrong, but I //think// it's saying run this test only if the current host architecture is i386?
> > > 
> > Additionally, what the reasoning behind making these tests `--noexec` - it looks like most JitLink tests are? I'm wondering what makes the code in these tests non-runnable if we're able to load it into memory? I can see how the generated code for one architecture might be straight up rubbish for some other architecture, and therefore it does not make sense to actually try and execute the generated code. But then why go to the point where we're loading things into memory even? Why not stop after generating the `LinkGraph`?
> > I may be wrong, but I think it's saying run this test only if the current host architecture is i386?
> 
> That check is saying "if this LLVM build isn't configured with i386 backend support". You want to keep it -- it tells us whether or not the tools can handle i386 assembly. :) 
> I can see how the generated code for one architecture might be straight up rubbish for some other architecture, and therefore it does not make sense to actually try and execute the generated code.

Yep -- we want to run this on all architectures, and that means not executing it when target arch != host arch. We just go one step further and never execute it at all.

> But then why go to the point where we're loading things into memory even? Why not stop after generating the LinkGraph?

We want to check that all relocations have been applied correctly. Originally this testing system was designed for RuntimeDyld, which had no exposed data structure that we could evaluate, hence the decision to inspect final linked memory instead. Now that we have JITLink we could inspect the graph, but the advantage of checking the final linked memory is that it verifies that linking worked end-to-end, including layout in target memory.

It wouldn't hurt to rethink all this test infrastructure. It was written in a hurry back in 2015 and barely touched since. That said it has been remarkably effective -- a cleanup and new functionality would be welcome (LinkGraph inspection? Maybe a more user-friendly expression language and robust parser), but we'd definitely want to keep some variation of the end-to-end scheme that we have.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131347/new/

https://reviews.llvm.org/D131347