[LLVMdev] RFC: llvm-shlib-test (Was: [llvm] r191029 - llvm-c: Make LLVMGetFirstTarget a proper prototype)

Hans Wennborg hans at chromium.org
Mon Oct 14 14:46:13 PDT 2013


On Mon, Sep 30, 2013 at 1:45 AM, Anders Waldenborg <anders at 0x63.nu> wrote:
> Attached is what I got thus far.
>
> What I'm struggling with is proper integration in build system. What
> is in there is just wild guesses from my side, both on autoconf and
> cmake variants. It would be great if someone with proper knowledge of
> the buildsystems could have a look.

I don't know if I qualify for that, but it seems to be working for me :)

> Also I'm not sure how to properly
> handle compilation on msvc - clearly "-std=c11 -Wstrict-prototypes" is
> not the options I should use in that case. (also the dup2 use, will
> that work on windows?).

I think we should just punt on compilation with msvc for now. When I
tried, it wouldn't even compile the headers in include/llvm-c/. I
think we should fix this, but your patch shouldn't be blocked on it.

We should try to avoid any inherently msvc-hostile code, though. Like
the dup2 stuff - can we change llvm-c to allow for dumping a module to
stdout instead of stderr? Redirecting the streams is kind of hacky
anyway.


More comments inline:

> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt
> @@ -40,6 +40,8 @@ add_llvm_tool_subdirectory(llvm-mcmarkup)
>
>  add_llvm_tool_subdirectory(llvm-symbolizer)
>
> +add_llvm_tool_subdirectory(llvm-c-test)

I think we should just wrap this in an "if (NOT MSVC)" clause for now.
This way we can get your patch landed, and then work on getting it and
llvm-c to build with msvc if that seems feasible. Right now for
example, msvc chokes on all the "static inline" in the header files.


> --- /dev/null
> +++ b/tools/llvm-c-test/calc.c
> @@ -0,0 +1,121 @@
> +/*===-- calc.c - tool for testing libLLVM and llvm-c API ------------------===*\
> +|*                                                                            *|
> +|*                     The LLVM Compiler Infrastructure                       *|
> +|*                                                                            *|
> +|* This file is distributed under the University of Illinois Open Source      *|
> +|* License. See LICENSE.TXT for details.                                      *|
> +|*                                                                            *|
> +|*===----------------------------------------------------------------------===*|
> +|*                                                                            *|
> +|* This file implements the --calc command in llvm-c-test. --calc reads lines *|
> +|* from stdin, parses them as a name and an expression in reverse polish      *|
> +|* notation and prints a module with a function with the expression.          *|
> +|*                                                                            *|
> +\*===----------------------------------------------------------------------===*/
> +
> +#include "llvm-c-test.h"
> +#include "llvm-c/Core.h"
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +typedef LLVMValueRef (*binop_func_t)(LLVMBuilderRef, LLVMValueRef LHS,
> +                                     LLVMValueRef RHS, const char *Name);
> +
> +static const LLVMOpcode binop[] = {['+'] = LLVMAdd,

Nice! But I suspect MSVC will have a problem with this :/

> +                                   ['-'] = LLVMSub,
> +                                   ['*'] = LLVMMul,
> +                                   ['/'] = LLVMSDiv,
> +                                   ['&'] = LLVMAnd,
> +                                   ['|'] = LLVMOr,
> +                                   ['^'] = LLVMXor};
> +
> +static void handle_line(char **tokens, int ntokens) {
> +  char *name = tokens[0];
> +
> +  LLVMModuleRef M = LLVMModuleCreateWithName(name);
> +
> +  LLVMTypeRef I64ty = LLVMInt64Type();
> +
> +  LLVMTypeRef Fty = LLVMFunctionType(
> +      I64ty, (LLVMTypeRef[1]) { LLVMPointerType(I64ty, 0) }, 1, 0);
> +
> +  LLVMValueRef F = LLVMAddFunction(M, name, Fty);
> +  LLVMValueRef param;
> +  LLVMBasicBlockRef BB = LLVMAppendBasicBlock(F, "entry");
> +  LLVMBuilderRef builder = LLVMCreateBuilder();
> +  LLVMPositionBuilderAtEnd(builder, BB);
> +
> +  LLVMGetParams(F, &param);
> +
> +  LLVMSetValueName(param, "in");
> +
> +  char *end;
> +
> +  LLVMValueRef stack[32];
> +  int depth = 0;
> +
> +  for (int i = 1; i < ntokens; i++) {

Ultra nit: llvm style likes preincrement better. Not sure how much
this makes sense in C context, but may be worth sticking to for
consistency.

> +    char tok = tokens[i][0];
> +    switch (tok) {
> +    case '+':
> +    case '-':
> +    case '*':
> +    case '/':
> +    case '&':
> +    case '|':
> +    case '^':
> +      if (depth < 2) {
> +        printf("stack underflow\n");
> +        return;
> +      }
> +      stack[depth - 2] = LLVMBuildBinOp(builder, binop[tok], stack[depth - 1], stack[depth - 2], "");
> +      depth--;
> +      break;
> +    case '@':
> +      if (depth < 1) {
> +        printf("stack underflow\n");
> +        return;
> +      }
> +      LLVMValueRef off = LLVMBuildGEP(builder, param, &stack[depth - 1], 1, "");
> +      stack[depth - 1] = LLVMBuildLoad(builder, off, "");
> +      break;
> +    default: {
> +      long val = strtol(tokens[i], &end, 0);
> +      if (end[0] != '\0') {
> +        printf("error parsing\n");
> +        return;
> +      }
> +      if (depth >= 32) {

Maybe break out 32 to a #define ?


> +++ b/tools/llvm-c-test/helpers.c
> @@ -0,0 +1,50 @@
> +/*===-- helpers.c - tool for testing libLLVM and llvm-c API ---------------===*\
> +|*                                                                            *|
> +|*                     The LLVM Compiler Infrastructure                       *|
> +|*                                                                            *|
> +|* This file is distributed under the University of Illinois Open Source      *|
> +|* License. See LICENSE.TXT for details.                                      *|
> +|*                                                                            *|
> +|*===----------------------------------------------------------------------===*|
> +|*                                                                            *|
> +|* Helper functions                                                           *|
> +|*                                                                            *|
> +\*===----------------------------------------------------------------------===*/
> +
> +#include "llvm-c-test.h"
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +void tokenize_stdin(void (*cb)(char **tokens, int ntokens)) {
> +  char line[1024];
> +  char *tokbuf[512];
> +
> +  while (fgets(line, sizeof(line), stdin)) {
> +    int c = 0;
> +
> +    if (line[0] == ';' || line[0] == '\n')
> +      continue;
> +
> +    while (c < 512) {

Use sizeof(tokbuf) instead of 512?

> +      tokbuf[c] = strtok(c ? NULL : line, " \n");
> +      if (!tokbuf[c])
> +        break;
> +      c++;
> +    }
> +    if (c)
> +      cb(tokbuf, c);
> +  }
> +}
> +
> +int redirect_stderr_to_stdout() {

Should this be redirect_stderr_to_stdout(void) ?

> +  int old;
> +  old = dup(STDERR_FILENO);
> +  dup2(STDOUT_FILENO, STDERR_FILENO);

Yeah, this won't work well on Windows.

If the purpose of these redirect_ functions is just to be able to dump
modules to stdout rather than stderr, maybe it's LLVMDumpModule that
should be fixed, or needs an alternative function?


> diff --git a/tools/llvm-c-test/include-all.c b/tools/llvm-c-test/include-all.c
> new file mode 100644
> index 0000000..a5f5742
> --- /dev/null
> +++ b/tools/llvm-c-test/include-all.c
> @@ -0,0 +1,31 @@
> +/*===-- include-all.c - tool for testing libLLVM and llvm-c API -----------===*\
> +|*                                                                            *|
> +|*                     The LLVM Compiler Infrastructure                       *|
> +|*                                                                            *|
> +|* This file is distributed under the University of Illinois Open Source      *|
> +|* License. See LICENSE.TXT for details.                                      *|
> +|*                                                                            *|
> +|*===----------------------------------------------------------------------===*|
> +|*                                                                            *|
> +|* This file doesn't have any actual code. It just make sure that all         *|
> +|* the llvm-c include files are good and doesn't generate any warnings        *|
> +|*                                                                            *|
> +\*===----------------------------------------------------------------------===*/
> +
> +#include "llvm-c/Analysis.h"
> +#include "llvm-c/BitReader.h"
> +#include "llvm-c/BitWriter.h"
> +#include "llvm-c/Core.h"
> +#include "llvm-c/Disassembler.h"
> +#include "llvm-c/ExecutionEngine.h"
> +#include "llvm-c/Initialization.h"
> +#include "llvm-c/LinkTimeOptimizer.h"
> +#include "llvm-c/Linker.h"
> +#include "llvm-c/Object.h"
> +#include "llvm-c/Target.h"
> +#include "llvm-c/TargetMachine.h"
> +#include "llvm-c/Transforms/IPO.h"
> +#include "llvm-c/Transforms/PassManagerBuilder.h"
> +#include "llvm-c/Transforms/Scalar.h"
> +#include "llvm-c/Transforms/Vectorize.h"
> +#include "llvm-c/lto.h"

I wonder if the build system could generate this list of files for us
somehow. Could probably just be a FIXME for now, though.

> +++ b/tools/llvm-c-test/module.c
> @@ -0,0 +1,131 @@
> +/*===-- module.c - tool for testing libLLVM and llvm-c API ----------------===*\
> +|*                                                                            *|
> +|*                     The LLVM Compiler Infrastructure                       *|
> +|*                                                                            *|
> +|* This file is distributed under the University of Illinois Open Source      *|
> +|* License. See LICENSE.TXT for details.                                      *|
> +|*                                                                            *|
> +|*===----------------------------------------------------------------------===*|
> +|*                                                                            *|
> +|* This file implements the --module-dump, --module-list-functions and        *|
> +|* --module-list-globals commands in llvm-c-test.                             *|
> +|*                                                                            *|
> +\*===----------------------------------------------------------------------===*/
> +
> +#include "llvm-c-test.h"
> +#include "llvm-c/BitReader.h"
> +#include "llvm-c/Core.h"
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>

Is unistd.h actually needed for this file?

> +
> +static LLVMModuleRef load_module(void) {
> +  LLVMMemoryBufferRef MB;
> +  LLVMModuleRef M;
> +  char *msg = NULL;
> +
> +  if (LLVMCreateMemoryBufferWithSTDIN(&MB, &msg)) {
> +    fprintf(stderr, "Error reading file: %s\n", msg);
> +    exit(1);
> +  }
> +
> +  if (LLVMParseBitcode(MB, &M, &msg)) {
> +    fprintf(stderr, "Error parsing bitcode: %s\n", msg);
> +    exit(1);
> +  }
> +
> +  return M;
> +}
> +
> +int module_dump(void) {
> +  LLVMModuleRef M = load_module();
> +
> +  int tmpfd = redirect_stderr_to_stdout();
> +  LLVMDumpModule(M);
> +  unredirect_stderr_to_stdout(tmpfd);
> +
> +  LLVMDisposeModule(M);
> +
> +  return 0;
> +}
> +
> +int module_list_functions(void) {
> +  LLVMModuleRef M = load_module();
> +  LLVMValueRef f;
> +
> +  f = LLVMGetFirstFunction(M);
> +  while (f) {
> +    if (LLVMIsDeclaration(f)) {
> +      printf("FunctionDeclaration: %s\n", LLVMGetValueName(f));
> +    } else {
> +      unsigned nisn = 0;
> +      unsigned nbb = 0;
> +
> +      printf("FunctionDefinition: %s [#bb=%u]\n", LLVMGetValueName(f),
> +             LLVMCountBasicBlocks(f));
> +
> +      for (LLVMBasicBlockRef bb = LLVMGetFirstBasicBlock(f); bb;
> +           bb = LLVMGetNextBasicBlock(bb)) {
> +        nbb++;
> +        for (LLVMValueRef isn = LLVMGetFirstInstruction(bb); isn;
> +             isn = LLVMGetNextInstruction(isn)) {
> +          nisn++;
> +          if (LLVMIsACallInst(isn)) {
> +            LLVMValueRef callee =
> +                LLVMGetOperand(isn, LLVMGetNumOperands(isn) - 1);
> +            printf(" calls: %s\n", LLVMGetValueName(callee));
> +          }
> +        }
> +      }
> +      printf(" #isn: %u\n", nisn);
> +      printf(" #bb: %u\n\n", nbb);
> +    }
> +    f = LLVMGetNextFunction(f);
> +  }
> +
> +  LLVMDisposeModule(M);
> +
> +  return 0;
> +}
> +
> +static const char *TypeKindNames[] =
> +    {[LLVMVoidTypeKind] = "VoidTypeKind",
> +     [LLVMHalfTypeKind] = "HalfTypeKind",
> +     [LLVMFloatTypeKind] = "FloatTypeKind",
> +     [LLVMDoubleTypeKind] = "DoubleTypeKind",
> +     [LLVMX86_FP80TypeKind] = "X86_FP80TypeKind",
> +     [LLVMFP128TypeKind] = "FP128TypeKind",
> +     [LLVMPPC_FP128TypeKind] = "PPC_FP128TypeKind",
> +     [LLVMLabelTypeKind] = "LabelTypeKind",
> +     [LLVMIntegerTypeKind] = "IntegerTypeKind",
> +     [LLVMFunctionTypeKind] = "FunctionTypeKind",
> +     [LLVMStructTypeKind] = "StructTypeKind",
> +     [LLVMArrayTypeKind] = "ArrayTypeKind",
> +     [LLVMPointerTypeKind] = "PointerTypeKind",
> +     [LLVMVectorTypeKind] = "VectorTypeKind",
> +     [LLVMMetadataTypeKind] = "MetadataTypeKind",
> +     [LLVMX86_MMXTypeKind] = "X86_MMXTypeKind", };

Again, I don't think MSVC will be happy. But with a switch-case
thingy, LLVM will generate the equivalent (some day it might even be
better) code :)


> --- /dev/null
> +++ b/test/Bindings/llvm-c/disassemble.test
> @@ -0,0 +1,29 @@
> +; RUN: llvm-as < %s | llvm-c-test --module-list-functions | FileCheck %s

I guess this test will only work for builds that include both the
x86_64 and arm targets, so the lit config file should probably take
that into account.

> +
> +
> +arm-linux-android    44 26 1f e5 0c 10 4b e2 02 20 81 e0

How does this work? I don't see how llvm-as would parse this as valid
LLVM IR, and this test fails locally for me.

> +++ b/test/Bindings/llvm-c/globals.ll
> @@ -0,0 +1,7 @@
> +; RUN: llvm-as < %s | llvm-shlib-test --list-module-globals | FileCheck %s

I guess this should be llvm-c-test --module-list-globals


> --- /dev/null
> +++ b/tools/llvm-c-test/llvm-c-test.h
> @@ -0,0 +1,39 @@
> +/*===-- llvm-c-test.h - tool for testing libLLVM and llvm-c API -----------===*\
> +|*                                                                            *|
> +|*                     The LLVM Compiler Infrastructure                       *|
> +|*                                                                            *|
> +|* This file is distributed under the University of Illinois Open Source      *|
> +|* License. See LICENSE.TXT for details.                                      *|
> +|*                                                                            *|
> +|*===----------------------------------------------------------------------===*|
> +|*                                                                            *|

All the other files had a nice comment here :)

> +|*                                                                            *|
> +|*                                                                            *|
> +\*===----------------------------------------------------------------------===*/
> +#ifndef _LLVM_C_TEST_H

Do we normally use leading _ for include gurads in LLVM hreaders?

> +#define _LLVM_C_TEST_H
> +
> +/* helpers.c */

These comments are pretty non-LLVM style. Not a big deal, but still.



More information about the llvm-dev mailing list