[PATCH] D16513: [Unittest] Clean up formatting, NFC

[David Blaikie via llvm-commits]

On Thu, Feb 4, 2016 at 7:39 PM, Joseph Tremoulet <jotrem at microsoft.com>
wrote:

> I replied to this in Phabricator via the web interface (
> http://reviews.llvm.org/D16513#inline-139715), but it doesn't seem to
> have generated e-mail, so here it is:
> ---
>
> > it might be simpler to a separate function, attributed with LLVM_UNUSED
> (so no warnings fire on it) rather than the volatile approach
>
> OK, I can do that.  The comment at the definition of LLVM_ATTRIBUTE_UNUSED
> mentions "Prefer cast-to-void wherever it is sufficient" because it's more
> portable, so I wonder: would it be better still to wrap this in a lambda
> and then just cast the closure to void?
>

That's probably overkill/not a great improvement over what you've already
got (& that's for the case where the function is called/executed - which
you don't intend/want here)


> > (also, I worry about API testing like this - if we have a formal layer
> concept we want to implement - we should be able to (& we should) check
> that independently of any particular implementation (& it should check the
> semantics as well - as we are testing above, presumably). To keep the tests
> isolated, to avoid slipping API boundaries, narrow tests, etc
>
> Yeah, it felt hokey adding a "just make sure this compiles" test.  I went
> forward with it because it is an improvement over the current state of
> things and I verified that it would have caught the regression that hit us
> downstream.  I'd be happy to be pointed in the direction of something more
> proper (e.g. is there some way to capture this as a static_assert over type
> traits or something?),


There would be, with all the usual ridiculous template metaprogramming,
etc...


> but I don't see what that would be.  You predicated the suggestion on "if
> we have a formal layer concept", and ISTM the issue here is that we have
> only an informal layer concept, because the formalism we want is precisely
> a `concept` describing object layers, but `concept`s haven't made it into
> the language yet.


A language level construct isn't a necessary feature of the formality I was
speaking of. I mean the same formality as, say, the C++ container libraries.

No one tests that std::copy works with std::list specifically, you just
test that std::list meets its interface (which is a narrower interface than
what std::copy accepts) and std::copy accepts generic iterators.


>   I think that our informal "concept" of an object layer here is
> "something that has the same signature as the ObjectLinkingLayer", which is
> why this test conjoins the ObjectTransformLayer being tested specifically
> to an ObjectLinkingLayer instance, and why there are no other similar tests
> (we don't, to my knowledge, have any other object layers in-tree).
>
> To make it more concrete, ObjectLinkingLayer and the informal "object
> layer" concept have a template method with this signature:
>
>    template <typename ObjSetT,
>              typename MemoryManagerPtrT,
>              typename SymbolResolverPtrT>
>    ObjSetHandleT addObjectSet(ObjSetT Objects,
>                               MemoryManagerPtrT MemMgr,
>                               SymbolResolverPtrT Resolver)
>
>
> This test was added with rL258630, which fixed a regression introduced by
> rL258185 adjusting the signature of the ObjectLinkingLayer (and the
> general/informal "object layer" "concept"), without updating the
> ObjectTransformLayer.  The signature change actually looks benign, as the
> change was of the Objects parameter from `const ObjSetT &Objects` to
> `ObjSetT Objects`, but the issue is that the ObjectTransformLayer's attempt
> to forward Objects then changed to involve a copy, and ObjSetT can be
> instantiated to a non-copyable type.  This was the best way I thought of to
> write a test that would catch that sort of change, but I'm happy for
> suggestions on a better way.
>

Yeah, more or less I think this should be tested in two ways:

Generic tests against each API, which probably would not have caught this
regression unless someone was smart enough to think about testing
ObjectLinkingLayer with a non-copyable type as that parameter. That's the
sort of level of unit testing the API probably should have, but even when
we aspire to it we don't always get there.

The other part is that we should have some end-to-end tests (which,
conveniently, are pretty cheap/fast and still "unit"-y in the sense of
LLVM's regression suite, so they can probably live there without too much
philosophical discord about test architecture/layering/etc)


> Lastly, I'm afraid I don't entirely understand what you mean regarding
> testing semantics when you say "check that independently of any particular
> implementation (& it should check the semantics as well - as we are testing
> above, presumably" -- the object layer concept, as I understand it, is
> essentially just a signature, so I'm not following what semantics we'd
> check in a way that's independent of any particular implementation.


Test it two fold: the implementation should be tested given a wide variety
of inputs that conform to its (rather broad) contract. The users should be
tester with a stub/mock that fails fast on anything outside that contract.

Sorry, perhaps I'm still being too vague... if/when I get time I can try to
work up a prototype/some patches.

- Dave


> The ObjectTransformLayer in particular has the semantics that it applies
> its transformation to objects passed in and forwards everything else to the
> base layer; yes that is what the code above is testing, but I'm not
> following how you're suggesting it be restructured.
>
>
> Thanks
> -Joseph
>
>
> -----Original Message-----
> From: David Blaikie [mailto:dblaikie at gmail.com]
> Sent: Wednesday, February 3, 2016 1:01 PM
> To: Joseph Tremoulet <jotrem at microsoft.com>; lhames at gmail.com
> Cc: llvm-commits at lists.llvm.org
> Subject: Re: [PATCH] D16513: [Unittest] Clean up formatting, NFC
>
> dblaikie added inline comments.
>
> ================
> Comment at: unittests/ExecutionEngine/Orc/ObjectTransformLayerTest.cpp:282
> @@ -281,56 +281,3 @@
>    volatile bool RunStaticChecks = false;
> -  if (RunStaticChecks) {
> -    // Make sure that ObjectTransformLayer implements the object layer
> concept
> -    // correctly by sandwitching one between an ObjectLinkingLayer and an
> -    // IRCompileLayer, verifying that it compiles if we have a call to the
> -    // IRComileLayer's addModuleSet that should call the transform layer's
> -    // addObjectSet, and also calling the other public transform layer
> methods
> -    // directly to make sure the methods they intend to forward to exist
> on
> -    // the ObjectLinkingLayer.
> -
> -    // We'll need a concrete MemoryManager class.
> -    class NullManager : public llvm::RuntimeDyld::MemoryManager {
> -    public:
> -      uint8_t *allocateCodeSection(uintptr_t, unsigned, unsigned,
> -                                   llvm::StringRef) override {
> -        return nullptr;
> -      }
> -      uint8_t *allocateDataSection(uintptr_t, unsigned, unsigned,
> -                                   llvm::StringRef, bool) override {
> -        return nullptr;
> -      }
> -      void registerEHFrames(uint8_t *, uint64_t, size_t) override {}
> -      void deregisterEHFrames(uint8_t *, uint64_t, size_t) override {}
> -      bool finalizeMemory(std::string *) override { return false; }
> -    };
> -
> -    // Construct the jit layers.
> -    ObjectLinkingLayer<> BaseLayer;
> -    auto IdentityTransform = [](
> -
> std::unique_ptr<llvm::object::OwningBinary<llvm::object::ObjectFile>>
> -            Obj) { return Obj; };
> -    ObjectTransformLayer<decltype(BaseLayer), decltype(IdentityTransform)>
> -        TransformLayer(BaseLayer, IdentityTransform);
> -    auto NullCompiler = [](llvm::Module &) {
> -      return llvm::object::OwningBinary<llvm::object::ObjectFile>();
> -    };
> -    IRCompileLayer<decltype(TransformLayer)> CompileLayer(TransformLayer,
> -                                                          NullCompiler);
> -    std::vector<llvm::Module *> Modules;
> -
> -    // Make sure that the calls from IRCompileLayer to
> ObjectTransformLayer
> -    // compile.
> -    NullResolver Resolver;
> -    NullManager Manager;
> -    CompileLayer.addModuleSet(std::vector<llvm::Module *>(), &Manager,
> -                              &Resolver);
> -
> -    // Make sure that the calls from ObjectTransformLayer to
> ObjectLinkingLayer
> -    // compile.
> -    decltype(TransformLayer)::ObjSetHandleT ObjSet;
> -    TransformLayer.emitAndFinalize(ObjSet);
> -    TransformLayer.findSymbolIn(ObjSet, Name, false);
> -    TransformLayer.findSymbol(Name, true);
> -    TransformLayer.mapSectionAddress(ObjSet, nullptr, 0);
> -    TransformLayer.removeObjectSet(ObjSet);
> -  }
> +  if (!RunStaticChecks)
> +    return;
> ----------------
> Also, it might be simpler to a separate function, attributed with
> LLVM_UNUSED (so no warnings fire on it) rather than the volatile approach.
>
> (also, I worry about API testing like this - if we have a formal layer
> concept we want to implement - we should be able to (& we should) check
> that independently of any particular implementation (& it should check the
> semantics as well - as we are testing above, presumably). To keep the tests
> isolated, to avoid slipping API boundaries, narrow tests, etc)
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160208/705ef7c9/attachment.html>