diff mbox

[avr] Fix mmcu to specs issues

Message ID 286bfa2f-84ba-beb2-1b5a-b92873a3ed91@microchip.com
State New
Headers show

Commit Message

Pitchumani Sivanupandi July 28, 2016, 11:50 a.m. UTC
On Tuesday 26 July 2016 06:00 PM, Georg-Johann Lay wrote:
> On 26.07.2016 12:20, Pitchumani Sivanupandi wrote:
>> avr-gcc expected to find the device specs in the search paths 
>> specified. But
>> it doesn't work as expected when device specs in different place than 
>> the
>> installed location.
>>
>> Example-1:
>> avr-gcc wrongly assumes the device specs will be in first identified
>> 'device-specs' folder in prefix path. avr-gcc natively supports device
>> at90pwm1, but complains as it couldn't find the specs file in the first
>> 'device-specs' directory in the search path.
>>
>>
>> $ avr-gcc test.c -mmcu=at90pwm1 -B /home/install/dev/atxmega128a1u/
>>
>> avr-gcc: error: cannot access device-specs for _at90pwm1_ expected at
>
> Are the "_"s literally? Then the spec file name should be 
> "specs-_at90pwm1_".
No, It was mangled character shown by my terminal for format specifier 
"%qs" in printf.
Ignore it.
...
>> Example-2:
>> Similar issue happens when -flto option is enabled and device specs 
>> in custom
>> search path.
>>
>> atxmega128a1u device specs in custom path and that is passed with -B 
>> option.
>> Architecture spec files such as specs-avrxmega7 will be in installed 
>> location.
>>
>> $ avr-gcc test.c -mmcu=atxmega128a1u -flto -B 
>> /home/install/dev/atxmega128a1u/
>>
>> avr-gcc: error: cannot access device-specs for _avrxmega7_ expected at
>> _/home/install/dev/atxmega128a1u/device-specs/specs-avrxmega7_
>> avr-gcc: note: supported core architectures: avr2 avr25 avr3 avr31 
>> avr35 avr4
>> avr5 avr51 avr6 avrxmega2 avrxmega4 avrxmega5 avrxmega6 avrxmega7 
>> avrtiny avr1
>> avr-gcc: note: you can provide your own specs files, see
>> <http://gcc.gnu.org/onlinedocs/gcc/Spec-Files.html> for details
>> lto-wrapper: fatal error: avr-gcc returned 1 exit status
>> compilation terminated.
>> /home/avr-trunk/install/lib/gcc/avr/7.0.0/../../../../avr/bin/ld: error:
>> lto-wrapper failed
>> collect2: error: ld returned 1 exit status
>>
>> Attached patch to address these issues.
>>
>> Fix:
>> Replace device-specs-file spec function with mmcu-to-device-specs. 
>> This will
>> not assume that specs files are in first identified device-specs 
>> directory.
>> Instead this spec function will let gcc identify the absolute path of 
>> specs file
>> using spec string "device-specs-file (device-specs/specs-<mcu>%s)".
>
> IIUC this leads to problems with LTO and when the install path 
> contains spaces which windows distros usually have.  The problem is 
> that the spec function cannot escape the spaces as it would need more 
> than 1 escape level.
>
> It might also be the case that -mmcu= is specified more than once 
> (with the same MCU), but I don't remember exactly in which situations 
> this happens... Doesn't this lead to inclusion of more than one 
> specs-file?

Yes, it lead to space problem.

Modified the patch because of path with spaces problem. When LTO 
enabled, multiple specs-file are
included as -mmcu=<arch> is fed back to gcc. It happens with/ without of 
this patch.
e.g. For atmega164p, device's and architecture's specs file given when 
invoking collect2.
  -specs=device-specs/specs-atmega164p -specs=device-specs/specs-avr5

Attached new patch. It just removes the conditions that led to 
originally stated issues.
(In driver-avr.c:avr_devicespecs_file)
Removed first condition as -mmcu=avr* shall come when LTO enabled. 
Second condition to
check absolute path is wrong as the specfile_name composed here will not 
be available
if the specs file is not present in first 'device-specs' directory.

With this approach, we can't issue 'descriptive' error for unavailable 
specs-file.
But, avr-gcc will issue below error if specs file is not found.
$ avr-gcc -mmcu=unknown test.c
avr-gcc: error: device-specs/specs-unknown: No such file or directory

Is that Ok?

Regards,
Pitchumani

gcc/ChangeLog

2016-07-28  Pitchumani Sivanupandi <pitchumani.s@atmel.com>

     * config/avr/driver-avr.c (specfiles_doc_url): Remove.
     (avr_diagnose_devicespecs_error): Remove.
     (avr_devicespecs_file): Remove conditions to check specs-file,
     let -specs= option handler do the validation.

Comments

Georg-Johann Lay July 29, 2016, 8:36 a.m. UTC | #1
On 28.07.2016 13:50, Pitchumani Sivanupandi wrote:
> On Tuesday 26 July 2016 06:00 PM, Georg-Johann Lay wrote:
>> On 26.07.2016 12:20, Pitchumani Sivanupandi wrote:
>>> avr-gcc expected to find the device specs in the search paths specified. But
>>> it doesn't work as expected when device specs in different place than the
>>> installed location.
>>>
>>> Example-1:
>>> avr-gcc wrongly assumes the device specs will be in first identified
>>> 'device-specs' folder in prefix path. avr-gcc natively supports device
>>> at90pwm1, but complains as it couldn't find the specs file in the first
>>> 'device-specs' directory in the search path.
>>>
>>>
>>> $ avr-gcc test.c -mmcu=at90pwm1 -B /home/install/dev/atxmega128a1u/
>>>
>>> avr-gcc: error: cannot access device-specs for _at90pwm1_ expected at
>>
>> Are the "_"s literally? Then the spec file name should be "specs-_at90pwm1_".
> No, It was mangled character shown by my terminal for format specifier "%qs" in
> printf.
> Ignore it.
> ...
>>> Example-2:
>>> Similar issue happens when -flto option is enabled and device specs in custom
>>> search path.
>>>
>>> atxmega128a1u device specs in custom path and that is passed with -B option.
>>> Architecture spec files such as specs-avrxmega7 will be in installed location.
>>>
>>> $ avr-gcc test.c -mmcu=atxmega128a1u -flto -B /home/install/dev/atxmega128a1u/
>>>
>>> avr-gcc: error: cannot access device-specs for _avrxmega7_ expected at
>>> _/home/install/dev/atxmega128a1u/device-specs/specs-avrxmega7_
>>> avr-gcc: note: supported core architectures: avr2 avr25 avr3 avr31 avr35 avr4
>>> avr5 avr51 avr6 avrxmega2 avrxmega4 avrxmega5 avrxmega6 avrxmega7 avrtiny avr1
>>> avr-gcc: note: you can provide your own specs files, see
>>> <http://gcc.gnu.org/onlinedocs/gcc/Spec-Files.html> for details
>>> lto-wrapper: fatal error: avr-gcc returned 1 exit status
>>> compilation terminated.
>>> /home/avr-trunk/install/lib/gcc/avr/7.0.0/../../../../avr/bin/ld: error:
>>> lto-wrapper failed
>>> collect2: error: ld returned 1 exit status
>>>
>>> Attached patch to address these issues.
>>>
>>> Fix:
>>> Replace device-specs-file spec function with mmcu-to-device-specs. This will
>>> not assume that specs files are in first identified device-specs directory.
>>> Instead this spec function will let gcc identify the absolute path of specs
>>> file
>>> using spec string "device-specs-file (device-specs/specs-<mcu>%s)".
>>
>> IIUC this leads to problems with LTO and when the install path contains
>> spaces which windows distros usually have.  The problem is that the spec
>> function cannot escape the spaces as it would need more than 1 escape level.
>>
>> It might also be the case that -mmcu= is specified more than once (with the
>> same MCU), but I don't remember exactly in which situations this happens...
>> Doesn't this lead to inclusion of more than one specs-file?
>
> Yes, it lead to space problem.
>
> Modified the patch because of path with spaces problem. When LTO enabled,
> multiple specs-file are
> included as -mmcu=<arch> is fed back to gcc. It happens with/ without of this
> patch.
> e.g. For atmega164p, device's and architecture's specs file given when invoking
> collect2.
>  -specs=device-specs/specs-atmega164p -specs=device-specs/specs-avr5
>
> Attached new patch. It just removes the conditions that led to originally
> stated issues.
> (In driver-avr.c:avr_devicespecs_file)
> Removed first condition as -mmcu=avr* shall come when LTO enabled. Second
> condition to
> check absolute path is wrong as the specfile_name composed here will not be
> available
> if the specs file is not present in first 'device-specs' directory.
>
> With this approach, we can't issue 'descriptive' error for unavailable specs-file.
> But, avr-gcc will issue below error if specs file is not found.
> $ avr-gcc -mmcu=unknown test.c
> avr-gcc: error: device-specs/specs-unknown: No such file or directory
>
> Is that Ok?

Looks reasonable, just ...


> Regards,
> Pitchumani
>
> gcc/ChangeLog
>
> 2016-07-28  Pitchumani Sivanupandi <pitchumani.s@atmel.com>
>
>     * config/avr/driver-avr.c (specfiles_doc_url): Remove.
>     (avr_diagnose_devicespecs_error): Remove.
>     (avr_devicespecs_file): Remove conditions to check specs-file,
>     let -specs= option handler do the validation.
>  const char*
>  avr_devicespecs_file (int argc, const char **argv)
>  {
> -  char *specfile_name;
>    const char *mmcu = NULL;
>
>  #ifdef DEBUG_SPECS
> @@ -111,12 +88,10 @@ avr_devicespecs_file (int argc, const char **argv)
>        break;
>      }
>
> -  specfile_name = concat (argv[0], dir_separator_str, "specs-", mmcu, NULL);
> -
>  #ifdef DEBUG_SPECS
>    if (verbose_flag)
> -    fnotice (stderr, "'%s': mmcu='%s'\n'%s': specfile='%s'\n\n",
> -             __FUNCTION__, mmcu, __FUNCTION__, specfile_name);
> +    fnotice (stderr, "'%s': mmcu='%s'\n\n",
> +             __FUNCTION__, mmcu);
>  #endif

... this DEBUG_SPECS makes hardly any sense any more because it doesn't print 
any specs-related info.

Johann
diff mbox

Patch

diff --git a/gcc/config/avr/driver-avr.c b/gcc/config/avr/driver-avr.c
index 83ca373..647f91b 100644
--- a/gcc/config/avr/driver-avr.c
+++ b/gcc/config/avr/driver-avr.c
@@ -29,41 +29,18 @@  along with GCC; see the file COPYING3.  If not see
 
 static const char dir_separator_str[] = { DIR_SEPARATOR, 0 };
 
-static const char specfiles_doc_url[] =
-  "http://gcc.gnu.org/onlinedocs/gcc/Spec-Files.html";
-
-
-static const char*
-avr_diagnose_devicespecs_error (const char *mcu, const char *filename)
-{
-  error ("cannot access device-specs for %qs expected at %qs",
-         mcu, filename);
-
-  // Inform about natively supported devices and cores.
-
-  if (strncmp (mcu, "avr", strlen ("avr")))
-    avr_inform_devices ();
-
-  avr_inform_core_architectures ();
-
-  inform (input_location, "you can provide your own specs files, "
-          "see <%s> for details", specfiles_doc_url);
-
-  return X_NODEVLIB;
-}
-
 
 /* Implement spec function `device-specs-fileĀ“.
 
-   Compose -specs=<specs-file-name>%s.  If everything went well then argv[0]
-   is the inflated (absolute) specs directory and argv[1] is a device or
-   core name as supplied by -mmcu=*.  When building GCC the path might
-   be relative.  */
+   Validate mcu name given with -mmcu option. Compose
+   -specs=<specs-file-name>%s. If everything went well then argv[0] is the
+   inflated (absolute) first device-specs directory and argv[1] is a device
+   or core name as supplied by -mmcu=*. When building GCC the path might be
+   relative.  */
 
 const char*
 avr_devicespecs_file (int argc, const char **argv)
 {
-  char *specfile_name;
   const char *mmcu = NULL;
 
 #ifdef DEBUG_SPECS
@@ -111,12 +88,10 @@  avr_devicespecs_file (int argc, const char **argv)
       break;
     }
 
-  specfile_name = concat (argv[0], dir_separator_str, "specs-", mmcu, NULL);
-
 #ifdef DEBUG_SPECS
   if (verbose_flag)
-    fnotice (stderr, "'%s': mmcu='%s'\n'%s': specfile='%s'\n\n",
-             __FUNCTION__, mmcu, __FUNCTION__, specfile_name);
+    fnotice (stderr, "'%s': mmcu='%s'\n\n",
+             __FUNCTION__, mmcu);
 #endif
 
   // Filter out silly -mmcu= arguments like "foo bar".
@@ -131,26 +106,12 @@  avr_devicespecs_file (int argc, const char **argv)
         return X_NODEVLIB;
       }
 
-  if (/* When building / configuring the compiler we might get a relative path
-         as supplied by "-B.".  Assume that the specs file exists and MCU is
-         a core, not a proper device then, i.e. we have "-mmcu=avr*".  */
-      (0 == strncmp (mmcu, "avr", strlen ("avr"))
-       && specfile_name[0] == '.')
-      /* vanilla */
-      || (IS_ABSOLUTE_PATH (specfile_name)
-          && !access (specfile_name, R_OK)))
-    {
-      return concat ("-specs=device-specs", dir_separator_str, "specs-", mmcu,
-                     // Use '%s' instead of the expanded specfile_name.  This
-                     // is the easiest way to handle pathes containing spaces.
-                     "%s",
+  return concat ("-specs=device-specs", dir_separator_str, "specs-",
+                 mmcu, "%s" 
 #if defined (WITH_AVRLIBC)
-                     " %{mmcu=avr*:" X_NODEVLIB "} %{!mmcu=*:" X_NODEVLIB "}",
+                 " %{mmcu=avr*:" X_NODEVLIB "} %{!mmcu=*:" X_NODEVLIB "}",
 #else
-                     " " X_NODEVLIB,
+                 " " X_NODEVLIB,
 #endif
-                     NULL);
-    }
-
-  return avr_diagnose_devicespecs_error (mmcu, specfile_name);
+                 NULL);
 }