[llvm-commits] [llvm] r94746 - in /llvm/trunk: lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp lib/Support/CommandLine.cpp lib/Target/PIC16/PIC16.h tools/bugpoint/ExtractFunction.cpp tools/llvm-ld/llvm-ld.cpp

[Jeffrey Yasskin]

Many of these seem like things the compiler should be doing for us.

On Thu, Jan 28, 2010 at 10:04 AM, Benjamin Kramer
<benny.kra at googlemail.com> wrote:
> Author: d0k
> Date: Thu Jan 28 12:04:38 2010
> New Revision: 94746
>
> URL: http://llvm.org/viewvc/llvm-project?rev=94746&view=rev
> Log:
> Replace strcpy with memcpy when we have the length around anyway.
>
> Modified:
>    llvm/trunk/lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp
>    llvm/trunk/lib/Support/CommandLine.cpp
>    llvm/trunk/lib/Target/PIC16/PIC16.h
>    llvm/trunk/tools/bugpoint/ExtractFunction.cpp
>    llvm/trunk/tools/llvm-ld/llvm-ld.cpp
>
> Modified: llvm/trunk/lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp?rev=94746&r1=94745&r2=94746&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp (original)
> +++ llvm/trunk/lib/ExecutionEngine/Interpreter/ExternalFunctions.cpp Thu Jan 28 12:04:38 2010
> @@ -368,7 +368,7 @@
>
>       switch (Last) {
>       case '%':
> -        strcpy(Buffer, "%"); break;
> +        memcpy(Buffer, "%", 2); break;

gcc and clang already make this optimization:

$ cat test.c
#include <string.h>
void foo(char*out) {
  strcpy(out, "%");
}
$ gcc -S test.c -o -
	.text
.globl _foo
_foo:
	pushl	%ebp
	movl	%esp, %ebp
	subl	$8, %esp
	movl	8(%ebp), %eax
	movw	$37, (%eax)
	leave
	ret

>       case 'c':
>         sprintf(Buffer, FmtBuf, uint32_t(Args[ArgNo++].IntVal.getZExtValue()));
>         break;
> @@ -400,8 +400,9 @@
>         errs() << "<unknown printf code '" << *FmtStr << "'!>";
>         ArgNo++; break;
>       }
> -      strcpy(OutputBuffer, Buffer);
> -      OutputBuffer += strlen(Buffer);
> +      size_t Len = strlen(Buffer);
> +      memcpy(OutputBuffer, Buffer, Len + 1);
> +      OutputBuffer += Len;

This seems like a missed optimization for both gcc and clang.

>       }
>       break;
>     }
>
> Modified: llvm/trunk/lib/Support/CommandLine.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CommandLine.cpp?rev=94746&r1=94745&r2=94746&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Support/CommandLine.cpp (original)
> +++ llvm/trunk/lib/Support/CommandLine.cpp Thu Jan 28 12:04:38 2010
> @@ -507,8 +507,9 @@
>
>   // Copy the program name into ProgName, making sure not to overflow it.
>   std::string ProgName = sys::Path(argv[0]).getLast();
> -  if (ProgName.size() > 79) ProgName.resize(79);
> -  strcpy(ProgramName, ProgName.c_str());
> +  size_t Len = std::min(ProgName.size(), size_t(79));
> +  memcpy(ProgramName, ProgName.data(), Len);
> +  ProgramName[Len] = '\0';

This one changes the semantics, although probably in a good way, so
the compiler can't help. It'd be nice to find a way to let the
compiler help here though, by asserting that there are no '\0's in the
string.

>   ProgramOverview = Overview;
>   bool ErrorParsing = false;
>
> Modified: llvm/trunk/lib/Target/PIC16/PIC16.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PIC16/PIC16.h?rev=94746&r1=94745&r2=94746&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Target/PIC16/PIC16.h (original)
> +++ llvm/trunk/lib/Target/PIC16/PIC16.h Thu Jan 28 12:04:38 2010
> @@ -55,9 +55,10 @@
>
>   // External symbol names require memory to live till the program end.
>   // So we have to allocate it and keep.
> +  // FIXME: Don't leak the allocated strings.
>   inline static const char *createESName (const std::string &name) {
>     char *tmpName = new char[name.size() + 1];
> -    strcpy (tmpName, name.c_str());
> +    memcpy(tmpName, name.c_str(), name.size() + 1);
>     return tmpName;
>   }
>
>
> Modified: llvm/trunk/tools/bugpoint/ExtractFunction.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/bugpoint/ExtractFunction.cpp?rev=94746&r1=94745&r2=94746&view=diff
>
> ==============================================================================
> --- llvm/trunk/tools/bugpoint/ExtractFunction.cpp (original)
> +++ llvm/trunk/tools/bugpoint/ExtractFunction.cpp Thu Jan 28 12:04:38 2010
> @@ -323,8 +323,6 @@
>  Module *BugDriver::ExtractMappedBlocksFromModule(const
>                                                  std::vector<BasicBlock*> &BBs,
>                                                  Module *M) {
> -  char *ExtraArg = NULL;
> -
>   sys::Path uniqueFilename(OutputPrefix + "-extractblocks");
>   std::string ErrMsg;
>   if (uniqueFilename.createTemporaryFileOnDisk(true, &ErrMsg)) {
> @@ -359,9 +357,8 @@
>   }
>   BlocksToNotExtractFile.close();
>
> -  const char *uniqueFN = uniqueFilename.c_str();
> -  ExtraArg = (char*)malloc(23 + strlen(uniqueFN));
> -  strcat(strcpy(ExtraArg, "--extract-blocks-file="), uniqueFN);
> +  std::string uniqueFN = "--extract-blocks-file=" + uniqueFilename.str();

This is actually a pessimization since it allocates an extra string.
It's more readable though.

> +  const char *ExtraArg = uniqueFN.c_str();
>
>   std::vector<const PassInfo*> PI;
>   std::vector<BasicBlock *> EmptyBBs; // This parameter is ignored.
> @@ -370,7 +367,6 @@
>
>   if (uniqueFilename.exists())
>     uniqueFilename.eraseFromDisk(); // Free disk space
> -  free(ExtraArg);
>
>   if (Ret == 0) {
>     outs() << "*** Basic Block extraction failed, please report a bug!\n";
>
> Modified: llvm/trunk/tools/llvm-ld/llvm-ld.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-ld/llvm-ld.cpp?rev=94746&r1=94745&r2=94746&view=diff
>
> ==============================================================================
> --- llvm/trunk/tools/llvm-ld/llvm-ld.cpp (original)
> +++ llvm/trunk/tools/llvm-ld/llvm-ld.cpp Thu Jan 28 12:04:38 2010
> @@ -179,8 +179,9 @@
>   // Make a copy of the list.  Don't forget the NULL that ends the list.
>   entries = 0;
>   while (envp[entries] != NULL) {
> -    newenv[entries] = new char[strlen (envp[entries]) + 1];
> -    strcpy (newenv[entries], envp[entries]);
> +    size_t len = strlen(envp[entries]) + 1;
> +    newenv[entries] = new char[len];
> +    memcpy(newenv[entries], envp[entries], len);
>     ++entries;
>   }
>   newenv[entries] = NULL;